mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-11-02 10:17:10 +01:00
ea1d84477ea16ea4d2acab74a4dc232360fe7c88
215
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris LuandDevin
|
3c1e8ca7a8 |
volume: never finish serving a needle whose data fails its CRC (#11467)
* storage: hold back last chunk until CRC verifies on whole-needle reads Above PagedReadLimit the needle is streamed: headers and body go out before the checksum is computed, so a corrupted needle was served as 200 with bad bytes and readers could not fall back to a replica. The final chunk is now written only after the checksum verifies; on a mismatch the response ends short of Content-Length and the client sees a failed transfer. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * rust volume: verify needle checksum on streamed reads (parity) Mirror the Go fix: carry the needle checksum in NeedleStreamInfo and have StreamingBody accumulate the CRC and verify it before emitting the last frame; a mismatch ends the body with an error so the client sees the transfer fail rather than receiving corrupt bytes that look complete. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * server: abort the transfer when a committed stream fails A writeFn failure after any byte or WriteHeader call leaves the declared status and Content-Length already sent; http.Error's text then joins the body and can exactly fill the withheld tail of a corrupted needle read — the client sees a complete 200 instead of a failed transfer to retry. Track whether the response is committed (headers sent, or bytes buffered for the deferred flush) and panic with http.ErrAbortHandler instead of appending an error body; pre-commit failures keep the 500 path. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * server: drop the response writer wrapper from the committed-response check Counting buffered writes is enough: with no bytes buffered the status and headers cannot have gone out, and the range branches commit via the explicit WriteHeader call before writeFn runs. The extra ResponseWriter wrapper added a new Write sink site that CodeQL flags. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
Chris LuandDevin
|
0f3ba98e11 |
volume: make volume.scrub report a live needle whose stored id is damaged (#11468)
* storage: scrub live needles' stored id against the index key scrubVolumeData only compared the needle's stored id for tombstones, so header damage on a live needle — where the data CRC cannot see it — passed every scrub mode while reads of that needle kept failing or serving the wrong key's data. Compare the id for every indexed needle. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * rust volume: scrub live needles' stored id against the index key (parity) Mirror the Go scrub fix: compare the stored needle id with the index key for live needles too, not only for deleted ones. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * rust volume: cover damaged live needle id in scrub test The tombstone test proved the index-key check fires for deleted entries; add the live-needle mirror of Go's TestScrubVolumeDataChecksLiveNeedleId so a regression in the live path is caught in Rust too. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
Chris LuandDevin
|
8ad2f29e3e |
shell: let volume.deleteEmpty drop volumes with no live needles (#11437)
* shell: let volume.deleteEmpty drop volumes with no live needles The candidate check only accepted a .dat at superblock size, so a volume whose every needle was deleted still had to be vacuumed first — minutes of compaction to rewrite bytes that were all garbage anyway. FileCount counts every indexed entry and DeleteCount every entry made garbage by overwrite or delete, so FileCount <= DeleteCount means nothing live remains and the volume can be unlinked directly. The quietFor guard is unchanged. * volume server: add only_garbage VolumeDelete guard VolumeDelete(only_empty) refuses every volume that ever held data, so a volume whose needles are all deleted could only be removed after a vacuum rewrote it. The new only_garbage flag deletes only when the byte counters show nothing live: DeletedSize covering all of ContentSize, the same all-garbage state vacuum measures. Byte counters are used because the file/delete counts drift on index reload. * rust volume: mirror only_garbage VolumeDelete guard Same check as the Go server: a volume deletes under only_garbage when its deleted bytes cover all content bytes. The grpc handler rejects before the store drops the volume from its map, since destroy errors after removal would still unmount it. * volume delete: let either enabled check pass, keep onlyEmpty on the wire An upgraded shell sending only_garbage to a pre-upgrade server would be read as an unconditional delete (field ignored, only_empty false). The request now keeps only_empty set so old servers check emptiness and refuse, while new servers delete when either check passes. * volume.deleteEmpty: skip remote-backed and protected read-only volumes A remote-tiered replica shares its cloud object with the other replicas, so keepRemoteData=false on one delete removes data they still reference. Protected read-only volumes are quarantined or under maintenance, which is exactly when a replica should not be dropped. * volume delete: validate guarded copies across disks before deleting * volume delete: hold copy locks across guarded validate-and-delete CheckVolumeDeletable released each copy's locks before Destroy ran, so a write landing on a later copy between the two passes refused its destroy after earlier copies were already removed. Pin every copy's dataFileAccessLock (and its location's volumesLock) across validation and removal so a refused delete leaves all copies intact. * volume delete: send deleted-volume notices after releasing locks A blocking send on a full DeletedVolumesChan under volumesLock can stall the heartbeat loop that drains it while it waits on the same locks. Collect the notices under the lock span and send after release. * pb: restore generated-file cosmetics to match the repo's protoc version Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
|
f7680cf812 |
volume: one long-lived tokio runtime for blocking tiered S3 reads (#11448)
* volume: one long-lived tokio runtime for blocking tiered S3 reads block_on_tier_future, behind read_range_blocking and delete_file_blocking, spawned an OS thread and built a fresh current-thread tokio runtime on every call, then tore the runtime down. On an S3-tiered volume that is once per needle read, per streamed 64 KiB chunk, per DatScanPlan record and per destroy. The SDK client's pooled HTTPS connections are driven by tasks on the runtime a request ran on, so each teardown dropped the pool and every call re-dialed and re-handshook TLS. A panic inside the SDK was also flattened to the fixed string "tier runtime thread panicked". Now one process-wide runtime (OnceLock, multi_thread, 2 workers named tier-io) drives all tier I/O; block_on_tier_future spawns onto it and parks the caller on an mpsc channel for the JoinHandle result. Blocking the caller is unavoidable (the storage layer is synchronous) and is what the old code did through thread::spawn().join(). Handle::block_on is not used because the wrappers are also reached from inside another runtime's worker, where it panics with "Cannot start a runtime from within a runtime". JoinError panics are downcast to &str/String and the payload is kept in the error. Tests cover runtime reuse (Handle::id equal across calls, thread name tier-io), calls from a std thread, from spawn_blocking, and directly from current-thread and multi-thread runtime contexts, and the panic payload. Against the old body 7 of 9 fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: return a tier runtime build failure instead of panicking Review follow-up. tier_runtime() expect'ed the runtime build, so an OS refusing threads panicked inside Volume::destroy (after the volume left the in-memory map, before its files were removed) and inside needle reads, bypassing their error paths. Keep the runtime in a Mutex<Option<Runtime>> behind tier_handle() -> Result<Handle, String>: a failed build is returned to the caller through block_on_tier_future's existing Result and is not cached, so a later call retries once the pressure is gone. The lock is held only while building. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: trim comments on the shared tier I/O runtime Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
|
4299fdf578 |
volume server: VolumeEcShardsDelete full teardown unloads every disk and keeps the shard gauge honest (#11446)
* volume server: VolumeEcShardsDelete full teardown unloads every disk and keeps the shard gauge honest
Go's VolumeEcShardsDelete full teardown calls vs.store.UnloadEcVolume in
the blanket path (weed/server/volume_grpc_erasure_coding.go:488) and
location.UnloadEcVolume in the generation-fenced path (:511): each disk
that had the volume registered drops it, closes its shard descriptors
and gives back its ec_shards gauge before the artifacts are unlinked.
The Rust handler used Store::remove_ec_volume / DiskLocation::remove_ec_volume
instead, which only remove the map entry. Store::remove_ec_volume also
stops at the FIRST disk holding the vid, so on a split-disk volume
(shards on several disks) the blanket teardown left the sibling disks'
EcVolume registered with open fds while the unlink loop deleted their
files underneath it: the heartbeat kept advertising shards whose files
were gone, the inodes stayed pinned by the open descriptors, and the
VOLUME_GAUGE{collection,"ec_shards"} never came back down. The fenced
path leaked the gauge and the descriptors the same way on the one disk
it wiped.
Both paths now use the unload_ec_volume helpers from #11413 (every disk
for the blanket teardown, the strictly-older disk for the fenced one),
and the two Status::internal messages name the disk directory like Go's
"... on %s: %w".
Regression tests build a two-disk store with the same vid mounted on
each disk (the SplitDiskEcFixture, which gains a collection knob so the
gauge read is isolated from parallel tests mounting under "") and assert
that a blanket teardown leaves no EcVolume registered on any disk and
returns the gauge to its pre-mount value, and that a fenced teardown
decrements the gauge for the older disk's shard while preserving the
newer disk. Both fail against the previous handler.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* volume server: trim comments on the EC full-teardown unload path
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
||
|
|
3e9fc9e75b |
volume: reject invalid -minFreeSpace and duration flags at startup like Go (#11447)
* volume: reject invalid -minFreeSpace and duration flags at startup like Go Symptom: the Rust volume server swallowed invalid values for three startup flags and ran with a limit the operator did not ask for. -minFreeSpace=150 became 150 bytes (Go: fatal, percent must be 0..=100), -minFreeSpace=10GiBx and -minFreeSpace=50B fell through to the 1% default (Go: fatal), and -inflightUploadDataTimeout=30sec, =abc or =30 silently became the 60 s default (Go: flag.Duration usage error, exit 2). Go's byte-size units (1,024MB, 42 mib, 100Ki, 1eb) were also not understood and mapped to 1%. Go reference: weed/util/minfreespace.go:73-106 (MustParseMinFreeSpace / ParseMinFreeSpace), weed/util/bytes.go:212-243 (ParseBytes and bytesSizeTable), weed/command/volume.go:226-227 (flag.Duration, i.e. time.ParseDuration grammar). Fix: the two inflight timeout Cli fields are now std::time::Duration with a clap value_parser that ports time.ParseDuration (units ns/us/µs/ μs/ms/s/m/h, fractions, bare 0, int64-nanosecond overflow), so clap prints a usage error naming the value and exits 2 at the same layer as Go's flag package. parse_min_free_spaces returns Result and is built from Go-parity parse_min_free_space / parse_bytes; resolve_config_with_env aborts with Go's "The value specified in -minFreeSpace not a valid value" message, the fail-fast pattern it already uses for --max, --index and --readMode. No new dependency. Negative durations and NaN percent are rejected explicitly (Go quirks with no sane Rust representation). Tests cover the Go fixtures from minfreespace_test.go and bytes_test.go, the duration grammar, and flag-level rejection through Cli::try_parse_from and resolve_config_with_env; the flag-level tests were confirmed to fail against the previous parser. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: say that -minFreeSpace commas separate directories, never thousands Review follow-up. parse_bytes keeps Go's ParseBytes thousands-comma support, but the flag is split on commas into per-directory entries before any entry is parsed (Go's MustParseMinFreeSpace does the same), so -minFreeSpace=1,024MB is the two entries 1 and 024MB. The doc comment and the single-entry tests advertised the comma form as if it were reachable from the flag. Document the split and add a flag-level test: two entries from parse_min_free_spaces, and resolve_config aborting on the directory-count mismatch with one -dir. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: trim comments on the strict flag parsing Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
|
83d44be0f3 |
volume: detect S3 not-found by typed SDK errors, not the "service error" string (#11444)
* volume: detect S3 not-found by typed SDK errors, not the "service error" string
remote_storage/s3.rs decided ObjectNotFound by matching the Display
output of an aws_sdk_s3 SdkError against "NoSuchKey" / "404" /
"NotFound". In the locked SDK (aws-smithy-runtime-api 1.11.6,
src/client/result.rs:487-497) that Display is a fixed string per
variant, "service error" for every S3 error, so ObjectNotFound was
unreachable: every missing remote object surfaced as
Other("s3 get object: service error") with the real cause discarded.
Go (weed/remote_storage/s3/s3_storage_client.go) uses typed checks:
HEAD (373-374): awserr.RequestFailure with StatusCode() == 404;
GET (436-437): awserr.Error with Code() == s3.ErrCodeNoSuchKey.
read_file now matches SdkError::ServiceError whose GetObjectError
is_no_such_key(); a bare 404 on GET stays a generic error, as in Go.
stat_file matches HeadObjectError::is_not_found() or a raw HTTP 404
status, Go's actual condition. Non-service errors fall through to
Other unchanged. Every SdkError message in s3.rs and s3_tier.rs is
formatted with DisplayErrorContext so the S3 error code and message
survive instead of "service error".
Six network-free unit tests drive the client through a canned
HttpClient (404 NoSuchKey, bare 404 on GET and HEAD, 404 with a
foreign body on HEAD, 403 AccessDenied on GET and HEAD). They need
aws-smithy-runtime-api as a dev-dependency; it is already in the lock
at a single version, so no new crates.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* volume: HEAD not-found is the raw 404 status alone, as in Go
Review follow-up. The HEAD arm also accepted the SDK's NotFound error
code on any status, so a 400 carrying <Code>NotFound</Code> became a
missing object. Go's stat looks only at RequestFailure.StatusCode() ==
404 (weed/remote_storage/s3/s3_storage_client.go:373); do the same. The
raw status still covers the body-less 404 the SDK turns into NotFound
and a 404 whose body names a foreign code. Regression test for the
non-404 NotFound body, which failed against the previous arm.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* volume: trim comments on the typed S3 not-found checks
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---------
Co-authored-by: franchb <ru.balloo@gmail.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
||
|
|
df4995b894 |
volume server: VolumeMarkReadonly answers NotFound when the volume vanished under the lock (#11443)
* volume server: VolumeMarkReadonly answers NotFound when the volume vanished under the lock
make_volume_readonly looked the volume up, notified the master (step 1),
then took the store write lock (step 2) and marked the volume only `if
let Some(..)`. When the volume left the store during step 1 -- a master
round trip, during which an unmount or a heartbeat expiry can land --
the missing else meant the RPC reported success for a volume the server
no longer has, and step 3 told the master again that it is read-only.
Go's Store.MarkVolumeReadonly (weed/storage/store.go) returns
"volume %d not found" when findVolume comes back nil, and
makeVolumeReadonly (weed/server/volume_grpc_admin.go) returns that error
before the step-3 notification. The Rust step 2 now does the same:
find_volume_mut(vid) -> Status::not_found("volume {vid} not found"), and
the `?` skips step 3, as it already did for a set_read_only_persist
failure. The scrub caller already matches NotFound to skip such a
volume instead of failing the whole report; it now actually gets it.
volume_mark_writable already returns NotFound under its write lock.
The regression test opens the step-1 window deterministically: step 1
awaits the current_master_url read lock, so the test holds its write
guard, lets make_volume_readonly park there after its own lookup
succeeded, unmounts the volume, then releases the guard. With no master
configured the notification is a no-op, so the write lock in step 2 is
the only place left that can notice the volume is gone.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* volume server: trim comments on the vanished-volume mark-readonly path
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
||
|
Chris LuandDevin
|
317e756b9a |
volume server: validate ext and collection in gRPC CopyFile/ReceiveFile (Rust) (#11451)
* volume server: validate ext and collection in gRPC CopyFile Port the Go-side checks (checkVolumeFileExtension, checkVolumeCollection) to the Rust volume server so a client-supplied collection or ext carrying a separator or ".." cannot fold a path outside the volume directory. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * volume server: validate ext and collection in gRPC ReceiveFile Same port on the write path: the file ReceiveFile creates is built from client-supplied fields, so reject traversal there too. Reported through the response error field, matching Go's SendAndClose. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
|
bb9942c646 |
volume server: sweep stale EC artifacts before VolumeEcShardsGenerate re-encodes (#11413)
* volume server: sweep stale EC artifacts before VolumeEcShardsGenerate re-encodes The Rust VolumeEcShardsGenerate went straight into write_ec_files: no unload of an already-mounted EC volume and no stale-artifact sweep. Only .ec00..ecNN on the encoding disk were truncated, so a retry could mix two encode runs. A stale N.ec03 left on a sibling disk survived, reconcile later mounted it against the new .ecx, and the new .vif made the encode_ts_ns identity guard pass, so reads served old-run bytes at new-run offsets. Mirror Go's VolumeEcShardsGenerate (#9880 / #9953): UnloadEcVolume on every disk, then removeStaleEcArtifacts on every disk location before encoding. remove_ec_volume_files_full_teardown already has removeStaleEcArtifacts' semantics (.ec00..ec31, .ecx/.ecj/.ecsum[.vN] in both the data and idx dirs, .vif only on a shard-only disk; never the source .dat/.idx), so reuse it. Add Store::unload_ec_volume, which unlike remove_ec_volume does not stop at the first disk and closes the descriptors so the unlink frees the inodes. The store write lock covers only unload + sweep, not the encode. The failure arm now also drops the generation-0 .ecsum, as Go's defer does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume server: wake the heartbeat after VolumeEcShardsGenerate unloads shards The pre-encode unload drops mounted EC shards from memory, but unlike every other unmount path it did not wake the heartbeat, so the master kept routing reads to shards this server no longer serves until the next pulse. Notify once the store lock is released, and before the sweep error propagates: a failed sweep has unloaded the shards too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume server: clean up encode artifacts when the .vif write fails too Go's shouldCleanup defer covers every error before the .vif commits, not just a failed encode. A serialize or write failure on the .vif left the fresh .ecNN/.ecx/.ecsum behind, which the next generate would have to rely on the new sweep to remove. Extract the cleanup and run it on the .vif error paths as well. * volume server: write the EC .vif atomically Go's SaveVolumeInfo writes a temp file, syncs it, and renames it over the target, so a failed write leaves the previous metadata intact and a read-only .vif fails the save. The direct fs::write truncated the file first, so a write or sync failure could leave an empty .vif even after cleanup_encode removed the generated shards. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> |
||
|
|
f0afcf904d |
volume: an EC volume needs an .ecx to mount, and a 0-byte stub never outranks a real index (#11415)
* volume: an EC volume needs a non-empty .ecx to mount Two gaps against Go in how the Rust volume server treats the .ecx. EcVolume::new mounted with no index at all. The per-shard VolumeEcShardsMount path picks the disk by shard file alone, so a shard whose .ecx was on no local directory still registered and was advertised to the master; every VolumeEcShardRead then failed with "ecx file not open", and add_shard's 0-byte guard was neutralised because ecx_file_size stayed 0. Go's NewEcVolume returns an error wrapping os.ErrNotExist. EcVolume::new now fails with NotFound, and Store::mount_ec_shard looks up the .ecx owner across all disks first (findEcxIdxDirForVolume) so a shard on a sibling disk of its index still mounts instead of turning into a hard failure. A 0-byte .ecx stub, as left by a failed EC distribute copy, counted as a valid index. Go requires Size() > 0 wherever the file steers a decision: HasEcxFileOnDisk, findEcxIdxDirForVolume, indexEcxOwners (shared by reconcile and mirror), and VolumeEcShardsCopy removes a copied 0-byte .ecx and fails the copy. Mirror each through one is_usable_ecx_file helper. NewEcVolume itself still accepts a lone 0-byte .ecx as a legitimate empty index, but prefers a non-empty copy, local directory first, over a stub in the other directory; the resolution in EcVolume::new now follows the same order. Tests that mounted EC volumes without any .ecx get a real fixture. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: mount_ec_shard tries every disk; reconcile ignores a 0-byte local .ecx mount_ec_shard returned the first disk's error, so an unusable shard copy (a 0-byte .ecNN left by an interrupted move) hid a good copy on the next disk. Like Go's MountEcShards, keep scanning: NotFound means "not this disk", any other failure is collected, and an all-disks-fail error names every disk tried. "No .ecx on any local disk" is now told apart from "shard not on this server". The orphan-shard reconcile took its locally-mirrored fast path whenever a local .ecx existed at all. A 0-byte stub there registered the shards against an empty index while the owner index skipped that same stub. Go gates the fast path on HasEcxFileOnDisk; do the same. ec_local_ecx_path loses its last production caller and becomes test-only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: match Go's mount error text and skip the owner stat on the owning disk MountEcShards in Go skips the HasEcxFileOnDisk stat when the disk's own directories already hold the .ecx, dedups a shared -dir.idx across locations in findEcxIdxDirForVolume, and reports "load failures" with the same wording. Also drop two issue-number references from comments. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> |
||
|
Eliah RusinandClaude Fable 5.1
|
c1ccbcda13 |
volume server: the write queue answers uploads with the needle's real ETag (#11414)
With SEAWEED_WRITE_QUEUE=1 every upload came back with ETag "00000000". The upload handler built the needle with Needle::default(), so its checksum was CRC(0), and handed a clone of it to the queue. The CRC was only computed in the write path, on the worker's clone, and WriteResult carries no checksum back, so n.etag() in the handler formatted the zero checksum. The direct path writes through &mut n and was correct. Compute the checksum in the handler while building the needle, the way Go's CreateNeedleFromRequest does, over the same bytes the write path hashes (the stored data, gzipped or not). The ETag and the has-name flag are read before the write, so the needle is moved into the queue instead of cloned, which also drops a full payload copy per queued upload. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
|
94a68fa9b9 |
volume: walk_index_file keeps row alignment across short reads (#11412)
* volume: walk_index_file keeps row alignment across short reads walk_index_file issued one Read::read per batch and decoded whatever came back. Read::read may legally return a short count that is not a multiple of the 17-byte entry size (FUSE and network filesystems, a BufReader whose capacity is not a multiple of 17). The split entry at the end of the batch was dropped with no carry and the next read started mid-entry, so every later row was decoded from misaligned bytes and fed to the index as a garbage key/offset/size. This function backs every in-memory index load. Go's WalkIndexFile is immune because it reads through io.ReaderAt, which returns a full buffer or an error. Fill the batch buffer until it is full or the reader reports EOF, retrying ErrorKind::Interrupted, and only then decode whole entries. Reads stay batched at ROWS_TO_READ entries. EOF semantics are unchanged and match Go: on io.EOF Go decodes the whole entries in the final buffer, ignores a trailing partial entry and returns nil. A torn final entry is still skipped without an error here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume: trim walk_index_file comments The batch-fill loop and the ShortReader test helper each carried a paragraph where a sentence suffices. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> |
||
|
ssshr-66andChris Lu
|
b9ad62fc16 |
[Volume] Keep DAT and index state consistent after async batch Sync failure (#11425)
* fix 11400 * persist failed-recovery quarantine and harden rollback - record the unavailable state in a .unavailable marker, fsync it, and re-arm it on load so a restart cannot serve an unverified pair - quarantine the volume so heartbeats stop advertising it - block MarkVolumeWritable while unavailable, rechecked under noWriteLock - fail every request of a failed batch, not only the succeeded ones - restore the needle map and truncate .dat on inline fsync rollback failure - add truncateIndex for the sorted-file needle map - mirror the fail-closed semantics in the Rust volume server * volume: erase rolled-back mappings instead of leaving tombstones A rolled-back batch or failed inline write used Delete() to undo a needle that did not exist beforehand, leaving a tombstoned map entry whose stale offset makes the next write to that needle fail reading a header that no longer exists. Add removeMapping/restoreMapping to the mappers so recovery erases entries that were absent before the batch and reinstates the exact prior offset/size for ones that were, including tombstones. The index row still goes through Delete so a replay forgets the needle. * volume: gate bulk readers on unavailable and fsync the marker's dir - fsync_dir(&self.dir) synced the volume dir's parent, not the dir holding .unavailable; pass the marker path so the create survives a host crash - export UnavailableError and check it in ReadAllNeedles, VolumeTailSender, VolumeIncrementalCopy, and IncrementalBackup so replica-sync paths cannot stream or append data from an unverified .dat/.idx pair; mirror on the Rust side via read_dat_slice, read_all_needles, dat_scan_plan, and the incremental-copy handler * volume: drop issue references from comments near touched code * volume: stop active scans when the volume becomes unavailable The stream entry-point checks ran once per RPC, so a volume quarantined by a failed recovery mid-scan kept serving data. Recheck availability per needle/chunk on the detached read paths: tail scan and heartbeat, read-all, incremental copy, incremental backup writes, and the Rust StreamingBody chunk reads. Rust incremental copy also rejects a quarantined volume before sync_to_disk touches the backend. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
d848b8ed00 |
rust volume: regression test for makeup_diff replay across a 32 GiB offset boundary (#11410) (#11427)
* rust volume: test makeup_diff replay across a 32 GiB offset boundary Issue #11410 corrupted a replayed write's index offset in Go's makeupDiff by patching only four of the five offset bytes. The Rust makeup_diff already encodes the whole offset through idx_entry_to_bytes and Offset::from_actual_offset; this adds the mirror of TestConcurrentWriteCrossesOffsetBoundary so a regression would fail here the same way it does under -tags=5BytesOffset on the Go side. Sparse-truncate the .dat to 64 GiB, compact, write, commit: the index offset must equal the .cpd size and the needle must stay readable through a second vacuum. Gated on the 5bytes feature since a 64 GiB .dat exceeds the 32 GiB range of 4-byte offsets. * rust volume: skip the offset-boundary replay test on Windows Windows set_len allocates the full 64 GiB extension instead of a sparse range, so the test fails with StorageFull on CI runners. Gate it to unix, where set_len leaves the extension unallocated. |
||
|
|
0f2ecb766f |
volume server: reject non-ASCII input instead of panicking (#11406)
* volume server: reject non-ASCII input instead of panicking
Three parsers sliced attacker-supplied strings by byte offset, so a
multi-byte character split inside itself and panicked the task:
- parse_needle_id_cookie took the last 8 bytes as the cookie and the
rest as the needle id. Reachable from VolumeServer.BatchDelete,
whose file_ids come straight off the wire as protobuf strings;
that handler already answers 400 per bad fid, so the guard turns a
panicked RPC into the error it was already written to return.
- TTL::read took the unit as the last byte and the count as
everything before it, so "?ttl=5<multi-byte>" split mid-character.
The HTTP upload path does TTL::read(..).ok() and drops an invalid
TTL; AllocateVolume maps the Err to InvalidArgument.
Both now reject non-ASCII up front. Hex and a digits-plus-unit TTL are
ASCII by definition, so no accepted input changes -- covered by tests
alongside the rejection cases.
The six response-* header overrides were inserted with
parse().unwrap(). They come from the query string, so
"?response-cache-control=%0Aevil" decodes to a value HeaderValue
rejects and the unwrap panicked the connection task,
unauthenticated. They now skip the override, matching the if-let the
chunked-response path in the same file already uses.
ReplicaPlacement::from_string was reported as a fourth site but is not
one: reaching chars[2] requires chars[0] and chars[1] to be ASCII
digits, which forces the padded string to be three single-byte
characters, so a multi-byte character always lands on a to_digit()
None first. Kept as a regression test rather than a change.
Each fix was confirmed against the unfixed code first: the parser
tests panic with "byte index N is not a char boundary", and the
integration tests panic at handlers.rs:1413 and ttl.rs:88.
Not a vector, contrary to the report: the HTTP request line. The path
is not percent-decoded before parsing, so "%C3%A9" stays ASCII and
fails the length check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* volume server: fall back to needle MIME when response-content-type is invalid
Skipping an unparseable override left the response without any
Content-Type because the override had already bypassed the normal MIME
selection. Also correct a test comment that described a chars[2] panic
which cannot be reached.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: chrislusf <chrislusf@users.noreply.github.com>
|
||
|
|
ca62d4297b |
volume: load the .ecj deletion journal in chunks, and repair a torn tail (#11408)
* volume: load the .ecj deletion journal in chunks, and repair a torn tail Two independent defects in the EC deletion journal's load path. 1. The loader issued one NEEDLE_ID_SIZE-byte positional read per entry. That is fine for a healthy journal -- kilobytes -- and pathological for a large one. A `.ecj` is semantically a SET of deleted needle ids but is written as an append-only log that nothing dedupes, and several paths append a peer's ENTIRE journal onto the local one (VolumeEcShardsCopy with copy_ecj_file, EC index recovery, and ec_decode's deliberate cross-holder merge), so a volume whose shards are repeatedly balanced between two servers grows the file without bound. Observed in production: 1.51 TB and 1.30 TB on the two holders of one 10+4 volume containing ~100 distinct ids. At that size the per-entry loop is ~188e9 syscalls, run synchronously while holding the deleted_needles write lock and before the HTTP port opens. The process sits at 100% of one core with a small RSS -- the set stays tiny because the ids repeat -- reading at a few MiB/s because 8-byte reads defeat readahead, logs nothing after "Adding storage location", and ignores SIGTERM. The master then unregisters every volume it holds and reads of them fail. 4.46 and 4.47 are both affected. Read in 1 MiB chunks and build into a local set, merging once at the end so the write lock is not held for the whole scan. Measured on a 256 MiB journal of 100 distinct ids: 33,554,500 syscalls -> 257, identical resulting set. 2. A torn tail silently corrupted later deletes. The journal handle is in append mode, so writes land at the physical end regardless of alignment. A trailing partial record therefore pushed every later append out of alignment: the loader skipped the partial bytes, but the next mount decoded them together with the leading bytes of the following entry, producing one garbage id and dropping the delete that came after the tear -- after acknowledging it. Truncate to a whole number of records at mount, before anything can append. The repair uses its own read+write (non-append) handle: on Windows, append(true) requests FILE_APPEND_DATA without FILE_WRITE_DATA (and .write(true) is subsumed by .append(true)), so SetEndOfFile through the journal handle fails with ERROR_ACCESS_DENIED. The same trap exists in journal_delete's recovery path, which calls set_len on the append handle to roll back a partial write whose sync failed. It is error-handled rather than fatal, so on Windows that rollback silently does not happen. Untouched here; worth a separate fix. Bounding the journal's growth needs compaction, which is deliberately not in this change: replacing the file under a store that can hold several EcVolume instances for one volume id requires coordinating with the other holders, and that belongs at the store layer. Sent separately. Tests: a journal spanning several read chunks loads every entry; a trailing partial record is ignored rather than panicking; a torn tail is truncated at mount and a delete taken afterwards survives a remount. * volume: roll back a failed .ecj append through a dedicated write handle The append handle lacks FILE_WRITE_DATA on Windows, so the set_len rollback after a failed sync silently did nothing and the journal could drift one record past deleted_needles. Same trap as the torn-tail repair in this file; fix it the same way. Also format the new tests. * volume: mirror chunked .ecj load and torn-tail repair in Go --------- Co-authored-by: chrislusf <chrislusf@users.noreply.github.com> Co-authored-by: Devin <devin@cognition.ai> |
||
|
|
4bb40732bb |
volume server: ReceiveFile loses bytes and hides fsync failures (#11407)
* volume server: ReceiveFile loses bytes and hides fsync failures
Three defects in one handler, all on the path that receives a pushed
.dat/.idx/.vif or EC shard:
- `f.write(&content)` never compared the return to content.len().
A short write (ENOSPC, NFS) counted only the bytes that landed,
so every later chunk was written at a shifted offset and the RPC
answered error: "" with a byte count that looked right. Go's
os.File.Write loops. Now write_all.
- `let _ = f.sync_all();` discarded EIO and answered success with
the full byte count. Go omits the check too, but
ReceiveFileResponse carries an `error` field and the caller
renames the staged file into place on success -- so a silent
fsync failure publishes a file whose data never reached the
platter. Flush and fsync failures are now reported.
- Both the per-chunk write and the final fsync were blocking
std::fs calls inside the async fn, on the runtime worker that is
also driving the stream. Switched to tokio::fs + BufWriter, the
shape `drain_copy_stream_to_file` in this same file already uses
and documents. The partial-file cleanup on the error path moves
to tokio::fs::remove_file for the same reason.
The handler had no test at all, which is how the short-write bug
survived. Added a round-trip over a real connection with ragged chunk
boundaries, asserting the bytes on disk and not only the reported
count -- a dropped or reordered chunk changes the file even when
bytes_written still adds up.
That test guards the rewrite; it does not reproduce the original
faults. ENOSPC and EIO need fault injection that this suite has no
harness for, so the short-write and fsync paths are argued from the
code, not demonstrated by a failing test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* volume server: remove the staged file on every ReceiveFile error reply
Flush and fsync failures returned early and left the partial .copying or
shard file behind, as did the pre-existing write-error path. Route all
response-level errors through one cleanup block, matching Go's
close-and-remove on a failed write.
* volume server: tighten ReceiveFile comments
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: chrislusf <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <devin@cognition.ai>
|
||
|
|
8ff2e0777e |
volume server: HTTP DELETE on a distributed EC volume (#11405)
* volume server: HTTP DELETE on a distributed EC volume
The delete handler validated the cookie with EcVolume::read_ec_shard_needle,
which reads only locally-mounted shards and errors "ec shard N not available
locally" for any interval held by a peer. Every Err was mapped to 500 and no
.ecj tombstone was appended, so on a standard 10+4 spread over 14 servers an
HTTP delete of an EC needle could not succeed. The GET path already goes
through read_ec_shard_needle_distributed.
Route the delete's read through the same distributed reader. It does a
local-first pass in its snapshot phase, so the all-shards-local case costs
what it did before, and no store guard is held across the await (the reader
takes its own; RwLockReadGuard is !Send).
Two smaller corrections fall out of the new return type:
- the reader reports both "needle not in the index" and "volume vanished
between the has_ec check and the snapshot" as Ok(None), which collapses
the old Some(Ok(None)) and None arms into one 404;
- an io::ErrorKind::NotFound now answers 404 rather than 500, matching the
GET path. Telling a caller to retry a delete that can never succeed was
half the bug.
The cookie check and its ordering before the journal append are unchanged.
Not addressed here: Rust journals the tombstone locally while Go routes it to
the primary shard holder. That is a separate behaviour change and belongs in
its own PR against the same issue-10 checkbox.
The regression test mounts 13 of 14 shards, leaving out the one holding the
needle's interval. The distributed reader seeds its Reed-Solomon buffers from
locally mounted siblings, so with >= 10 survivors it reconstructs with no peer
fan-out -- which makes the bug reproducible on a single node. Against the
unfixed handler the test fails with 500 vs 202.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* volume server: fail the delete when the EC volume unmounts mid-request
find_ec_volume_mut returning None used to fall through to a 202 with no
.ecj tombstone written, reporting success for a delete that did not
happen. Answer 404 like the other volume-vanished arms so the caller can
retry after a remount.
* volume server: forward EC needle deletes to a primary-shard holder
Mirror Go's doDeleteNeedleFromAtLeastOneRemoteEcShards: the tombstone is
journaled on one holder of the needle's primary data shard via
VolumeEcBlobDelete (or the local journal when this server holds the
shard), falling back to any other shard holder when the primary has
none. Journaling only on the node that received the DELETE scattered
tombstones across whichever server took the request.
* volume server: route BatchDelete EC deletes through the same forwarding
BatchDelete had the same local-journal divergence as HTTP DELETE, plus a
gap the old code admitted in a comment: the .ecx index cannot supply the
needle's cookie, so EC deletes ran with no cookie check at all. A
distributed read now fills the needle for every EC entry — matching Go's
DeleteEcShardNeedle, which reads and compares the fid cookie even when
skip_cookie_check is set — and the tombstone forwards via
delete_ec_shard_needle_distributed. A needle deleted between read and
journal reports 304 like Go's ErrorDeleted; a vanished volume reports
500 so the filer retries.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: chrislusf <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <devin@cognition.ai>
|
||
|
Eliah RusinandClaude Fable 5.1
|
ac876eef21 |
rust volume: build outgoing gRPC clients in one place and give every dial a connect timeout (#11354)
server/grpc_client.rs stopped at build_grpc_endpoint() -> Endpoint, so all 13 production call sites hand-wrote the same .connect() + X::with_interceptor() + two max_*_message_size() lines. Four of them -- VolumeCopy, VolumeTailReceiver, VolumeEcShardsCopy and the HTTP chunk batch-delete fan-out -- dialed with no timeout at all, so an unreachable peer whose TCP handshake never completes (SYN dropped, blackholed route, host behind a silent firewall) left the operation waiting on the kernel's own retry budget, minutes long. Add GrpcDialOptions (unary / long / stream presets), connect_channel(), and volume_server_client() / master_client() / filer_client() constructors that attach the request-id interceptor and lift both message-size limits, then route all 13 sites through them. build_grpc_endpoint is private again, so connect_channel is the only way out of the module and no call site can dial without picking up a bound. Each site's existing timeouts are preserved exactly; the four bare dials gain a 5 s connect timeout and nothing else. No per-request deadline was added to any streaming call: Endpoint::timeout is a per-request bound on time-to-first-response-headers for every request the channel carries, so a value picked for one short call would also be the header deadline for the whole-volume transfer sharing the dial. The new bound covers the TCP handshake only -- tonic hands connect_timeout to HttpConnector::set_connect_timeout. A peer that completes the handshake and then stalls in the TLS or HTTP/2 exchange is still unbounded at those four sites, as are the RPCs themselves. That is why the three ping_* helpers keep their outer tokio::time::timeout: replacing it with connect_timeout would have narrowed a whole-connect bound they already had. main.rs no longer re-declares GRPC_MAX_MESSAGE_SIZE and the three keepalive/window constants; it imports them from grpc_client.rs so the inbound server and the outgoing clients cannot drift apart. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
|
818f3bb71b |
rust volume: share the I/O-error tracker between Volume and EcVolume (#11351)
* rust volume: share the I/O-error tracker between Volume and EcVolume Volume and EcVolume each carried the same three fields - a mutex-held last error, a consecutive count and a sticky quarantine flag - and the same four methods over them, identical except for the path qualifier on is_storage_io_error. The tolerance the count is compared against was a fourth copy: heartbeat.rs held VOLUME_IO_ERROR_TOLERANCE for volumes, ec_volume.rs held IO_ERROR_TOLERANCE for EC, and the volume test helper open-coded the same 3, so the two paths could drift apart silently. Go keeps this in one place already: weed/storage/io_error.go holds IoErrorTracker, IoErrorTolerance and isStorageIoError, and Volume embeds the tracker. Go's EcVolume has to re-implement it only because those fields are unexported and EC lives in another package. storage::io_error::IoErrorTracker now owns that state, with record / state / should_quarantine / mark_quarantined / reset and the single IO_ERROR_TOLERANCE. is_storage_io_error moves into the same file, so it sits with the tracker that is now its only caller, the way io_error.go is laid out. Both volume kinds embed one tracker and keep their existing method names as delegates, so the ~16 internal call sites and the readers in heartbeat.rs, store.rs and grpc_server.rs change only where the two threshold comparisons become should_quarantine(). Volume::last_io_error and EcVolume::reset_io_error_state had no callers and are gone. Unchanged: what counts as a storage-media error - is_storage_io_error changed file, not body, and is still the single predicate both volume kinds share, where Go's EcVolume tests EIO directly and so misses the Windows codes. Also unchanged: the tolerance value, the metric increment on every counted error, and the sticky quarantine - a success clears the count and the last error but never the flag, which only reset lifts. In the heartbeat the state read moved inside the quarantine branch, so the common path no longer takes the tracker's mutex or clones the last-error string; should_quarantine's two relaxed loads run either way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * rust volume: hoist absolute_display_path into server handlers.rs and ui.rs each held a byte-identical copy of the helper that turns a configured -dir into an absolute path for display. The status JSON and the status page are meant to show the same directory, so the two copies had to be edited together to stay that way. The helper now lives in server/mod.rs as pub(crate) and both callers use it. No behaviour change: same body, same call sites. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * rust volume: keep EcVolume::reset_io_error_state Moving both volume types onto the shared IoErrorTracker dropped EcVolume's public reset while Volume kept its own, so the two sides of the tracker drifted apart. mark_quarantined is sticky: a later successful read clears the error count through record(), but the quarantine flag only comes down through reset(). Without the delegate an EC volume that hit sustained media errors could not be returned to service in place once the storage was repaired. Go exposes the same method as EcVolume.ResetIoErrorState (weed/storage/erasure_coding/ec_volume.go:114). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * rust volume: name the shared tracker after Go's IoErrorTracker - check_read_write_error, get_io_error_state, mark_io_quarantined, reset_io_error_state match weed/storage/io_error.go one to one - io_error module is pub(crate) like the io module beside it - restore EcVolume::reset_io_error_state so both volume kinds expose the same recovery surface - trim comments that restate the code Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
|
7643f4f541 |
rust: a seaweed-common crate for the address and TLS helpers both crates carry (#11358)
* rust: a seaweed-common crate for the address and TLS helpers both crates carry seaweed-volume and seaweed-worker are separate cargo trees with separate lockfiles and no root manifest, so anything both of them need has had to be written twice. Two of those copies are a correctness risk rather than a typing cost, and this crate is where they stop being copies. address.rs is the HTTP<->gRPC port rule: `host:port` means gRPC on port+10000, `host:port.grpcPort` names it outright. The two copies had already drifted — the worker's bracketed IPv6 literals, the volume server's did not — so the rule lives here once, returning a typed AddressError whose Display text is the volume server's original wording, with join_host_port public beside it. A test asserts two of those messages in full rather than by substring, because the wording is the contract its callers hand to a Status or an io::Error; the other three end in a std ParseIntError message, which is std's to reword. The enum is #[non_exhaustive] so a future variant is not a breaking change for either consumer. The tests are both crates' cases together, plus the IPv6, already-bracketed and normalisation cases neither copy covered on its own. tls.rs is install_default_crypto_provider. Both binaries link aws-lc-rs and ring transitively, so rustls cannot auto-select and tonic's client TLS panics on first use; each binary has to pin one and it has to be the same one, which is exactly the kind of choice that should not exist twice. It is safe to share because `cargo tree -i rustls` resolves a single rustls in each tree (0.23.37 in seaweed-volume, 0.23.43 in seaweed-worker) and cargo unifies all semver-compatible `rustls = "0.23"` requirements into one crate per binary, so this crate writes the same process-wide static its consumer reads. rustls is already in both graphs — directly in the volume server, through tonic's tls-aws-lc in seaweed-worker-core — so the dependency adds no crate to either. rust-version is 1.91.1, the lower of the two consumers' floors, so depending on this crate cannot raise either tree's MSRV; verified with `cargo +1.91.1 check --all-targets`. The lockfile is committed even though this is a library: CI builds it directly, so a committed lock is what makes those runs reproducible and their caches stable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * rust: take the address and TLS helpers from seaweed-common Both public signatures are kept, so no caller outside the two wrapper files changes. parse_grpc_address stays `Result<String, String>` and maps the typed error through Display; server_to_grpc_address stays `Option<String>` and drops it with .ok(). Their doc comments and the volume server's 13 call sites are otherwise untouched. Three behaviours change, each in the direction of the copy that was already right: - The volume server now brackets IPv6 literals. `::1:19333` used to come back as `::1:29333`, which build_grpc_endpoint rejects with "invalid gRPC endpoint http://::1:19333: invalid authority" — an IPv6 master or EC peer could not be dialled at all. Two tests in grpc_client.rs pin it, one on the string and one on the endpoint the string builds. - The volume server now emits the *parsed* gRPC port of the dotted form instead of the original text it had just validated, so `host:8080.018080` and `host:8080.+18080` come back as `host:18080` rather than as authorities the URI parser rejects. Same port either way; only malformed spellings change. - The worker's dotted form now validates the HTTP port it discards. `server_to_grpc_address("host:abc.18080")` used to answer Some("host:18080"); it now answers None, which is what the volume server's copy has always done. install_default_crypto_provider becomes a re-export in both trees, so `crate::security::tls::install_default_crypto_provider` and `weed_lance_worker::tls::install_default_crypto_provider` still resolve. The lance crate's `rustls = "0.23"` was its only direct use of rustls and goes away with the body; seaweed-common states the same requirement, so neither the resolved version nor the enabled features move in either lockfile. The PEM test fixtures stay where they are. The two tests that use them are not duplicates: the volume server's exercises build_grpc_endpoint, and the lance one exists precisely because aws-lc-rs and ring are both linked in that crate's graph. Only the literals are shared, and exporting test fixtures from a library to dedupe two constants costs more than it saves. A path dependency outside both trees means every build context that copies one crate directory has to copy the other. The repo has one: the Rust source-build stage of docker/Dockerfile.go_build, which now copies seaweed-common beside seaweed-volume. Every workflow whose `paths:` filter keys on a crate directory gains `seaweed-common/**` — the two Rust test workflows, rust_binaries_dev, container_dev and performance. The tag- and dispatch-triggered ones (rust_binaries_release, container_release_unified, container_latest) have no `paths:` filter and need nothing. The two Rust test workflows also run `cargo test` in seaweed-common, from their unit-test job, because a path dependency is not a workspace member and neither tree's own `cargo test` reaches it. Each step builds into its job's cached target directory, and both cache keys now hash seaweed-common/Cargo.lock as well so a change there invalidates the cache it would otherwise silently reuse. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docker: keep go_build working for BRANCH revisions without seaweed-common The rust_builder stage copies seaweed-common unconditionally now that seaweed-volume path-depends on it, but BRANCH can name any revision — including ones that predate the crate. Create the directory in the builder stage so the COPY always has a source; an empty dir beside an old seaweed-volume is harmless. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
|
f1ed270942 |
rust volume: one S3 tier registry instead of two kept in sync by hand (#11357)
* rust volume: one S3 tier registry instead of two kept in sync by hand `VolumeServerState.s3_tier_registry` and `global_s3_tier_registry()` held the same S3 tier backends. `apply_storage_backends` — the only production writer — registered every backend into both, and each half of the tiering code then read a different one: the gRPC tier-move handlers resolved the backend from the per-server field, while `Volume`'s remote mount and destroy paths resolved it from the global registry, because a `Volume` has no handle to the server state. Two registries that must agree, kept in agreement by a duplicated `register_s3_backend` call and a comment in a test constructor explaining the hand-sync. Delete the field and let both tier-move handlers resolve from the global registry, so `apply_storage_backends` registers once and no longer needs the server state at all. Injecting a registry handle through `VolumeSpec` instead was considered and rejected here: it would touch every `Volume` constructor for no functional gain, and the process-wide registry is what `Volume` already uses. Behaviour is unchanged: the same names were registered in both registries, so every lookup resolves exactly as before. The tier-down test now registers its backend only in the global registry — before this change it fails with `remote storage s3.tier_down_delete not found from supported: []`. The tier-up handler had no test at all, so it gets a cheap probe: register a backend only in the global registry, ask for that destination, and check the call gets past the lookup — the response is dropped straight away, so the transfer sees a departed caller and never opens a connection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * rust volume: await the tier-up probe terminal error instead of racing it Dropping the response left it to chance whether the detached transfer saw the closed channel before its initial check; if it won that race it went on to attempt the multipart upload with no one waiting on the outcome. Hold the stream and read until the dead endpoint fails the upload — the terminal error proves the task ran and finished, so no background network work outlives the test. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Eliah RusinandClaude Fable 5.1
|
7dbbdac030 |
rust volume: keep the EC shard-location map, its refresh time and stale mark under one lock (#11356)
The per-EcVolume shard-location cache was three fields under three locks: an RwLock<HashMap> for the map, a Mutex<Option<Instant>> for the time it was last refreshed, and a Mutex<bool> for the stale mark. Nothing tied them together. merge_shard_locations published the merged map, released the write lock, and only then stamped the refresh time; both readers (scrub_ec_volume_distributed's snapshot and build_snapshot) took the two guards one after the other. A reader landing between the two writes paired a freshly merged map with the previous lookup's timestamp -- and that pair is exactly what needs_refresh judges, so a read went back to the master for a map that had just been refreshed. Go keeps the same state in one struct behind one ShardLocationsLock. replace_shard_locations documented itself as "a single observable step" while being two. Fold the three fields into one ShardLocationCache behind a single RwLock. merge_shard_locations upserts and stamps in one write section, shard_locations_snapshot returns the map and its time from one read section, and mark_shard_locations_stale / claim_shard_locations_refresh move the mark's read-and-consume onto the cache. The three zero-caller accessors -- set_shard_locations, replace_shard_locations, get_shard_locations -- are deleted, and the field is now private, so the invariant cannot be sidestepped from outside the module. The two test seeding sites go through merge_shard_locations, which already produces the state they were writing by hand. Unchanged: the freshness rule. needs_refresh keeps its thresholds and still judges the caller's snapshot -- the map that caller will actually read from, not whatever is cached by the time the claim runs -- so only the stale mark is read from under the new lock. The master lookup, the completeness guard in write_back_shard_locations and the per-shard upsert semantics are untouched. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
|
6d676eda67 |
rust volume: typed errors for store compaction so gRPC can answer NotFound (#11355)
* rust volume: typed errors for store compaction so gRPC can answer NotFound The vacuum entry points on `Store` returned `Result<_, String>`, so the gRPC layer had nothing to branch on and answered `Status::internal` for every failure. A vacuum loop that races a volume being moved or deleted saw the same code as a disk going bad, and `weed shell` could only tell the two apart by matching on the message text. `VolumeError` gains `VolumeNotFound(VolumeId)` — the existing `NotFound` is needle-level and carries no payload — and `InsufficientSpace`, and `compact_volume`, `commit_compact_volume`, `cleanup_compact_volume` and `delete_collection` return it. `impl From<VolumeError> for tonic::Status` in `server/mod.rs` maps not-found to `not_found`, read-only to `failed_precondition`, insufficient space to `resource_exhausted`, already-exists to `already_exists`, and everything else to `internal`; the four RPCs prefix their own context with `status_with_context`, so a message reads "commit compact volume 7: volume id 7 is not found". The store-side "during compact" / "during commit compact" / "during cleaning up" suffixes are gone, and the free-space message drops the volume id the prefix already supplies. `check_compact_volume` had no callers — `VacuumVolumeCheck` computes the garbage level from its own `find_volume` — and is deleted. `compact_volume` folded the size estimate into its first lookup, dropping the `unwrap()` re-lookup that only existed to dodge a borrow. `ascending_visit` on `CompactNeedleMap`, `RedbNeedleMap`, `SortedFileNeedleMap` and the `NeedleMap` dispatch is now generic over the visitor's error type, like `CompactMap::ascending_visit` already was. The three signatures that can fail on their own bound `E: From<String>` to carry those failures; the in-memory walk in `iter_entries` names `Infallible`, which says in the type what its comment used to say in prose. No Go shell command matches on the old error text: the strings exist only in weed/storage/store_vacuum.go. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * volume server: trim comments and answer the same codes from Go - vacuum_volume_check reports VolumeError::VolumeNotFound like the other vacuum RPCs instead of its own "not found volume id" wording - drop doc comments that restate what the code says - Go volume server wraps ErrVolumeNotFound/ErrInsufficientSpace from store_vacuum.go so VacuumVolumeCheck/Compact/Commit/Cleanup and DeleteCollection answer NotFound/ResourceExhausted, matching the Rust volume server; volumeDeleteStatusError generalized to volumeStatusError Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * volume server: prefix operation context on vacuum errors Lower-level errors forwarded by CompactVolume, CommitCompactVolume, CommitCleanupVolume and DeleteCollection carry no volume id or operation name. Wrap with %w so the status mapping still sees the sentinel chain, matching the context the Rust server's status_with_context adds. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * volume server: map NotEmpty to FailedPrecondition, share mapper in VolumeDelete Go's volumeStatusError maps ErrVolumeNotEmpty to FailedPrecondition; the Rust Status conversion was missing it and volume_delete kept a hand-rolled match. Route it through status_with_context like the vacuum handlers. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
Eliah RusinandClaude Fable 5.1
|
d002481037 |
rust volume: derive has_remote_file instead of mirroring it (#11353)
Volume carried `pub has_remote_file: bool` next to `pub volume_info`, and the bool was only ever the answer to `!volume_info.files.is_empty()`: outside the two constructors, `refresh_remote_write_mode` was the single writer. Both fields being public made the pair a convention rather than an invariant. Every caller that touched `volume_info.files` — load_vif twice, the tier-up handler, the tier-down handler and its rollback — had to remember to call `refresh_remote_write_mode` afterwards, and a caller that forgot would leave the volume advertising a write mode its .vif contradicts, or serving a remote .dat through a writable needle map. The bool becomes `has_remote_file()`, computed from the list, so it cannot drift. `volume_info` becomes private with a `volume_info()` reader, and edits to the reference list go through `update_remote_files(|files| ...)`, which applies the closure and then refreshes the derived write mode and the needle map. With no caller left outside the module, `refresh_remote_write_mode` is private. Unchanged: the refresh logic itself, the order of operations in both tier handlers, and the tier-down rollback semantics. The rollback still snapshots the removed reference before the refresh runs, restores it on failure, and re-refreshes unconditionally on the error path — the second `update_remote_files` call runs with a no-op closure when there was nothing to restore, exactly as the old code re-ran the refresh whether or not it had re-inserted a reference. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
Eliah RusinandClaude Fable 5.1
|
1df8c05bc3 |
rust volume: document every unsafe block and stop mutating the process env in tests (#11352)
Three production `unsafe` blocks carried no `// SAFETY:` comment at all (`libc::fallocate`, `libc::sysinfo`, `libc::statvfs`), and nothing made that an error: `clippy::undocumented_unsafe_blocks` is a `restriction` lint, allow-by-default, and appeared nowhere in either crate. Turn it on in `seaweed-volume`'s `[lints.clippy]` and in the worker workspace's `[workspace.lints.clippy]`, then document what each block relies on. `memory_status.rs` and `disk_location.rs` get their blocks narrowed to the `zeroed()` and the libc call, so each comment sits next to the operation it justifies and the arithmetic is outside the block. Both turn the success test into an early return on failure; the casts, the multiplication order and the values returned on either path are unchanged. The bigger problem was in `config.rs`'s tests. `with_temp_env_var` and `with_cleared_security_env` called `std::env::set_var`/`remove_var`, claiming soundness because every caller holds `process_state_lock()`. That mutex only serialises the fourteen annotated tests in this module. The same lib test binary runs the `grpc_server.rs` tests, which bind a `TcpListener`, dial loopback and drive a multi-thread tokio runtime, and tonic/hyper/rustls/aws-sdk all read the environment lazily on those threads — which is exactly the race Rust 2024 made these calls unsafe for. `restore_env_var` had no SAFETY comment at all. `#[serial]` would not have helped: it serialises annotated tests, which the mutex already did. So the config layer no longer reads the environment implicitly. An `EnvLookup<'a> = &'a dyn Fn(&str) -> Option<OsString>` is threaded from the public entry points down to every reader — `HOME`, `USERPROFILE`, the twenty-four `WEED_*` keys and `SEAWEED_WRITE_QUEUE`. `parse_cli` and `parse_security_config` keep their signatures and pass `process_env`, a thin wrapper over `std::env::var_os`; `resolve_config` becomes `resolve_config_with_env` (private, one caller). Tests build one with `fake_env` instead, so no test touches the real environment and every `unsafe` in the module is gone. `process_state_lock()` stays, with a smaller job: `set_current_dir` is safe but still process-global, so the tests that move the working directory are still serialised against the ones that read it. Tests naming an explicit config file never reach that search and no longer take the lock. No production behaviour changes: the same keys are read in the same order with the same precedence, and `env_string` reproduces `std::env::var(key).ok()` — absent and non-UTF-8 both read as unset. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
hsdfat
|
110b485bae |
fix(volume): stop ScanVolumeFileFrom at a header it cannot advance past (#11398)
fix(volume): stop scans at a header they cannot advance past A corrupt .dat header with a very negative size gives a record length (NeedleHeaderSize + NeedleBodyLength) of zero or less: v3 sizes -43..-36 and v2 sizes -35..-28 give exactly zero, and smaller sizes give a negative length. ScanVolumeFileFrom advanced by that length, so it re-read the same header forever or stepped back into the record before it. weed fix, weed export, weed compact, incremental weed backup and the tail sender behind volume.move and volume.merge could hang on such a volume, and weed compact could also finish with a .cpx that had dropped every needle after the header. Return an error wrapping needle.ErrorCorrupted instead. The check runs after the visitor has seen the record, so the rebuild scanner still stops quietly with io.EOF. Smaller negative sizes whose record length is positive are still stepped over, preserving the salvage behavior compaction relies on. Mirror the guard into the Rust volume scans: DatScanPlan::scan and read_all_needles fail on a non-positive record length, as does scan_dat_head, so a corrupt header cannot stall a tail pass or leave the repair scan walking stale offsets. |
||
|
hsdfatandChris Lu
|
06dda12e4b |
fix(volume): validate sizes in ReadNeedleBlob and WriteNeedleBlob (#11399)
* fix(volume): reject negative sizes in ReadNeedleBlob and WriteNeedleBlob A ReadNeedleBlob RPC with a size of -44 or below (-36 on v2 volumes) panics in makeslice inside needle.ReadNeedleBlob. The volume gRPC server has no recovery interceptor, so one request kills the process. Smaller negative sizes return bytes that are not a record. WriteNeedleBlob accepted a negative size whenever the blob header carried the same value: it appended the blob to .dat and indexed the needle with that size, which reads as deleted. Reject size < 0 in both Volume methods. Size 0 still passes, since delete records carry it. The Rust volume server got the same storage guards in #11345. * fix(volume): reject needle blobs whose length does not match their size WriteNeedleBlob appends the blob as is. A blob that is not the length its size implies leaves .dat off the 8-byte grid, and every later ordinary write to the volume is indexed at a truncated offset and reads back as EOF. A blob off by 8 bytes keeps the grid but leaves bytes that a .dat scan reads as the next record. The in-tree callers already send exact lengths. The one case this newly refuses is a copy between volumes of different needle versions, and that case already writes a broken record: a v3 record lands on a v2 volume with 8 extra bytes, and a v2 record on a v3 volume either fails the timestamp check or lands 8 bytes short. This is separate from the negative-size guards, whose Rust counterpart is #11345. The Rust server does not check the length yet. * fix(volume): guard the blob buffer allocation in needle.ReadNeedleBlob Volume.ReadNeedleBlob rejected negative sizes, but needle.ReadNeedleBlob still sized its buffer from the size and is called directly by vacuum and other paths. Reject a deletion marker before make() there too, and use size.IsDeleted() in the volume-level checks. * fix(volume): mirror the blob length check in the rust volume server write_needle_blob_and_index checked the size against the blob header but appended the blob verbatim, so a blob that is not the length its size implies still leaves .dat off the record grid. Match the Go check. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
ssshr-66andChris Lu
|
cd1e738422 |
[Volume] Scrub local deletion tombstones during FULL scrub (#11396)
* fix 11388 * fix(volume): scrub validates local deletion tombstones TombstoneFileSize (-1) is an .idx-only sentinel; the physical record it points at carries a zero-sized body. Normalize deleted index sizes to 0 via onDiskSize before computing disk usage and calling ReadData, so corrupted or truncated tombstone records are detected instead of skipped. Offset-zero entries (remote logical deletes, no .dat record) remain skipped, and the physical needle id is checked against the index key. Mirror the behavior in the Rust volume server. * fix(volume): scrub preserves physical size of deleted non-tombstone entries Size.Raw()/raw() already encodes the index-to-disk mapping: tombstone (-1) -> 0, other negative sizes -> their absolute value (the offset then points at the original record, per the ReadDeleted path). Use it instead of mapping every deleted size to 0. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
37bf1cd91d |
volume: validate copy/tail source addresses before dialing (#11390)
* pb: stop exiting the process on malformed server addresses ServerToGrpcAddress and GrpcAddressToServerAddress called glog.Fatalf when hostAndPort could not parse the port, which os.Exit(255)ed the whole process. A caller-supplied copy or tail source address reached this path synchronously in the serving goroutine, so one anonymous VolumeCopy with a non-numeric port terminated the volume server. Log the parse error and return the input unchanged instead: the dial or request that consumes the address then fails as an ordinary error. * volume: validate copy and tail source addresses before dialing VolumeCopy, VolumeEcShardsCopy and VolumeTailReceiver dial a caller-supplied source address (SourceDataNode / SourceVolumeServer) with no endpoint validation, so an anonymous caller could aim the volume server at loopback, link-local (cloud metadata) or other unintended destinations and read dial behavior back as a connectivity oracle. Apply the same peer-target deny list FetchAndWriteNeedle uses for replica targets: the source must be a bare host:port whose host is not loopback, link-local or unspecified; cluster peers stay reachable on private networks, and -volume.allowUntrustedRemoteEndpoints opts out. The loopback-using copy tests set the flag to keep exercising the copy path in process. * rust volume: validate copy and tail source addresses before dialing Mirror the Go guard on the Rust volume server: volume_copy, volume_ec_shards_copy and volume_tail_receiver dial a caller-supplied source address, so run it through validate_replica_target first (bare host:port; no loopback, link-local or unspecified hosts; private peers stay allowed). --volume.allowUntrustedRemoteEndpoints opts out; the test fixture and the Rust test-cluster launcher set it so loopback sources in tests keep working. * volume: pin validated copy/tail source addresses at dial time validateReplicaTarget resolves the source hostname once, but the gRPC client resolved it again at connect, leaving a DNS-rebinding window for hostname sources. The copy and tail source dials now run through the same guardedDialerPolicy the remote-storage path uses, so every resolved address is re-checked against the replica deny list (private peers allowed) immediately before the TCP connect. guardedDialerPolicy also moves to util.OutboundDialContext so the guarded path keeps the -ip.bind source binding the default gRPC dialer had. The Rust volume server mirrors this with connect_guarded, a tonic connector that resolves, re-checks each address, and connects to the first passing IP; handlers use it whenever the untrusted-endpoint opt-out is off. A handler-level test now exercises the enabled validation branches for all three source-taking RPCs. * pb: return empty server address for malformed grpc addresses GrpcAddressToServerAddress used to return the unparseable input on a hostAndPort failure, so a malformed raft address (e.g. "host:abc") flowed into admin dashboard master maps unchanged. Return an empty string instead, skip empty conversions at the two raft-cluster merge sites, and drop the now-stale comment about the fatal exit the earlier commit removed. * test: opt erasure-coding loopback clusters out of the remote endpoint guard The erasure-coding suites drive VolumeEcShardsCopy / VolumeCopy between volume servers bound to 127.0.0.1, which the copy/tail source guard now rejects by default. Pass -volume.allowUntrustedRemoteEndpoints to the test volume launches, matching what the volume_server framework harnesses already do. * admin: only claim fallback master leadership on an empty raft response A nonempty RaftListClusterServers response whose entries were all rejected left masterMap empty, so the fallback marked the reachable current master as leader the same way a genuinely empty (non-raft) response does. Track whether the successful response returned zero servers and only promote the fallback master then. |
||
|
Chris Lu
|
ce1e0dc30a |
s3api: don't delete chunks when CreateEntry outcome is ambiguous (#11376)
* s3api: map ambiguous filer transport errors to retryable 503 Canceled, DeadlineExceeded and Unavailable can be returned after the filer applied the write, so the outcome is ambiguous. Reporting them as a 4xx tells the client not to retry; report ServiceUnavailable instead. * s3api: verify entry existence before deleting orphaned chunks A failed CreateEntry can still have landed on the filer when the error is a transport failure, and entryCreated=false would tombstone chunks a live entry references, leaving a dangling pointer that survives only because reads pass readDeleted=true until vacuum reclaims the needle. Before deleting, look the entry up: if it is stored with the same chunks, the write succeeded; if the lookup cannot be answered, keep the chunks for vacuum to reclaim; only a confirmed absence still cleans up. * s3api: regression tests for ambiguous CreateEntry outcomes Covers the three post-create-failure cases in putToFiler: the entry landed despite the error (treat as success, keep chunks), the entry is confirmed absent (delete orphans), and the outcome is unverifiable (keep chunks, return error). * volume: count reads served from deleted needles A readDeleted read succeeding on a tombstoned needle is the signal that metadata still points at deleted data. Count it under a readDeletedNeedle handler label in both the Go and Rust volume servers so the condition is visible before vacuum turns it into a 404. * s3api: never delete chunks on an ambiguous create error Review feedback on the first fix showed verification could still go wrong in both directions: a stale or lagged lookup could report not-found for a committed entry, a prefix object stores its chunks on a directory entry, and filer-side manifestization rewrites the top-level chunk ids the comparison relied on. Rework the rule so the outcome classes are asymmetric: - A transport-level error (anything filerErrorToS3Error maps to a retryable 503) is ambiguous and never deletes chunks; the lookup can only upgrade the write to success. - Any other error is a definitive filer refusal and still cleans up. confirmCreateLanded asks the write owner first, resolves the stored entry through chunk manifests, requires an exact match of the uploaded file ids, and on success runs the finalize callback the failed create skipped (under the object write lock, with the same rmObject undo the create path uses). Zero-chunk writes stay ambiguous since they cannot be told apart by chunks. * s3api: cover definitive refusals and stale entries in put tests The confirmed-failure case now uses a definitive refusal so it still exercises orphan cleanup, and a new case keeps chunks when the stored entry belongs to an older object rather than this PUT. * volume: count deleted-needle reads once per request Streamed Go reads ran the deleted check in readNeedle and again in readNeedleDataInto, and non-streamed Rust reads in stream_info and the full-read fallback, double-counting one request. Count at the single entry probe each implementation takes per GET: readNeedle in Go, read_needle_stream_info in Rust. * s3api: run recovered-write rollback under the object lock Two follow-ups from review: ResolveChunkManifest returns traversed manifest blobs in its manifestChunks output, so requiring it empty rejected every manifestized landing; and the rmObject undo ran after the object write lock was released, so a concurrent newer write could be deleted between finalize failure and rollback. Compare only the resolved data chunks and keep the undo inside the lock. * s3api: verify, finalize and roll back recovered creates in one lock A lookup done before the object write lock let a concurrent PUT replace the entry between the chunk comparison and the finalize/rollback section, so a failed afterCreate could rmObject a newer write. Run the owner lookup, manifest resolution, chunk comparison, afterCreate and the conditional undo inside a single withObjectWriteLock section. |
||
|
Eliah RusinandClaude Fable 5.1
|
799c495226 |
rust volume: one positional read helper; never seek a dup'd handle on Windows (#11342)
* rust volume: one positional read helper; never seek a dup'd handle on Windows
Positional read-exact was hand-rolled four times: the complete
cross-platform version in needle_map/sorted_file.rs, a Windows-only half
in volume.rs whose unix half was inlined as a
cfg(unix)/cfg(windows)/compile_error! triple at three call sites, a
byte-identical Windows-only copy in ec_volume.rs, and read_full_at in
ec_bitrot.rs. Three more sites -- EcVolumeShard::read_at,
EcLocalShard::read_at and ec_encoder::read_at_most -- hand-rolled the
short-read-permitted variant with a cfg(not(unix)) arm that
try_clone()s the handle and seeks it.
That last arm is wrong. A duplicated descriptor shares one kernel file
offset with the original, so seek-then-read is two syscalls against
state another thread can move in between: a concurrent reader or an
append repositions the offset and the read returns bytes from somewhere
else entirely. EcLocalShard::read_at documents that it must never seek,
one line above the seek. Windows seek_read carries its own offset in a
single call, so that window does not exist.
All seven now go through storage::io::{read_exact_at, read_at}, whose
module doc records why duplicating a handle is not a way to get a
private file position -- opening the file again is, as
Volume::dat_scan_plan already does. read_at_most keeps its own
fill-until-EOF loop; only the per-iteration positional read changes.
Behaviour on unix is unchanged: every unix arm was already
FileExt::read_exact_at or FileExt::read_at. The one exception is
ec_bitrot::verify_shard_blocks, which now retries on EINTR (std's
read_exact_at does; the loop it replaces did not) and, on unix, reports
the standard "failed to fill whole buffer" text instead of "short read
on shard block". The Windows arm still says "unexpected EOF in
seek_read"; both carry ErrorKind::UnexpectedEof, as before.
NeedleStreamSource::read_exact_at and Volume::read_exact_at_backend keep
their signatures; only their bodies shrink.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* rust volume: retry Interrupted in Windows read_exact_at
Unix std's FileExt::read_exact_at ignores ErrorKind::Interrupted and
retries, but the Windows seek_read loop propagated it, so the shared
exact-read contract differed by platform. seek_read can surface
ERROR_OPERATION_ABORTED, which std maps to Interrupted.
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
||
|
Eliah RusinandChris Lu
|
0eb638f503 |
fix(ec): BatchDelete cookie fail-closed via locate_data geometry (#11348)
* fix(ec): BatchDelete cookie fail-closed via locate_data geometry * fix(ec): honor skip_cookie_check, require full cookie header * fix(ec): retry short cookie header reads, still fail closed on EOF * chore(ec): trim cookie validation comments --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Eliah Rusin
|
caf3d157e6 |
fix(ec): encode drops tombstoned needles, last-wins replay (#11347)
* fix(ec): encode drops tombstoned needles, last-wins replay * fix(ec): drop zero-offset rows in encode, match readNeedleMap |
||
|
Eliah Rusin
|
def25ca84d | fix(ec): validate ShardId at gRPC boundary, reject >=32 (#11346) | ||
|
Eliah Rusin
|
701e397337 | fix(volume): reject negative Size, recover poisoned store lock (#11345) | ||
|
Eliah Rusin
|
4fc9ada2ec | ci: run seaweed-volume unit tests on Windows (#11349) | ||
|
Eliah RusinandClaude Fable 5.1
|
a73ba3adbb |
rust volume: parse vid/fid paths once; the proxy redirect drops the extension like Go (#11341)
handlers.rs split needle URLs in three places and the three disagreed. Go does it once, in parseURLPath (weed/server/common.go:218-249), and dispatches on the slash count: /vid/fid/filename takes the extension off the filename and leaves the fid whole, /vid/fid takes it off the fid, and the comma form splits the last segment on its last comma and dot. Two of the Rust copies got that wrong: - extract_file_id returned the path unchanged when it found no comma, so a JWT fid claim, which Go compares against vid + "," + fid for every URL form (volume_server_handlers.go:361-364), could never match a slash-form request. With a JWT key configured, every read, write or delete of /3/01637037d6 was a 401. - build_proxy_request_info's slash branch had no extension handling, so a redirect for /3/01637037d6.jpg sent the client to /3,01637037d6.jpg. Go's proxyReqToTargetServer formats "%s/%s,%s" from the already-stripped fid (volume_server_handlers_read.go:128-137) and so emits /3,01637037d6. The peer still serves either form, since the comma form strips the extension again, so this one is parity rather than breakage. Replace all three with one parse_needle_path returning vid, fid, ext and filename borrowed from the path. The fid keeps its _delta suffix, as in Go: parse_needle_id_cookie applies it and the JWT check strips it. The leading slash stays optional, so chunk manifest fids still parse. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
|
563c729e70 |
rust volume: stream the tail scan and release the store lock (#11275)
* rust volume: add a .dat scan plan that runs without the store lock DatScanPlan captures a fresh .dat handle, the version, the start offset and an end bound while the caller holds a store guard, then visits one record at a time with positional reads that never touch the Volume, the way Go's ScanVolumeFileFrom feeds a scanner. The handle pins the inode the offset was resolved against: a vacuum commit renames .cpd over .dat and destroy unlinks it, and neither rewrites the pinned bytes. The end bound is read while no writer can hold store.write(), so the scan never meets a partial append. It is a fresh open, not try_clone, because on Windows read_exact_at uses seek_read, which moves a cursor a clone shares with the writer. A header whose size is negative, or does not fit before the end bound, ends the pass before the body length is computed or anything is allocated. In today's scan a negative size reaches needle_body_length and either overflows the buffer size or walks the scan from a wrong offset. A size near i32::MAX overflows padding_length's i32 arithmetic, which panics in debug builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VF7E9SHPihG1jC1grU9H3 * rust volume: stream the tail scan with the store lock released volume_tail_sender read every needle from the start offset to EOF into a Vec while holding store.read(). volume.merge tails from zero, so that was the whole volume in memory. And because needle writes and the heartbeat take store.write() on a lock that prefers writers, the whole node stopped serving until the scan finished: the failure #11235 fixed for EC scrub. Each pass now runs on a blocking thread. Under one store guard it resolves the start offset and captures a DatScanPlan, then drops the guard and sends each needle as it is read, as Go's VolumeFileScanner4Tailing does. This replaces the one-guard-across- search-and-scan rule from the previous commit with a stronger invariant: the offset, the handle and the end bound come from the same guard, and the handle pins the inode, so a vacuum commit mid-scan cannot point the offset into the compacted file. A scan error now ends the stream with Status::internal instead of a clean EOF, as Go's `streamFollow: %w` does. Once needles stream, a clean EOF after a partial pass would let volume.move treat a truncated tail as complete. A panic in the pass is reported the same way. A receiver that hangs up is also noticed between skipped needles, not only on a send. Unchanged: the append_at_ns filter, the header on every 2MB chunk, the caught-up heartbeat without a scan, and the draining countdown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VF7E9SHPihG1jC1grU9H3 * rust volume: fail the tail pass on a short read below the snapshot end DatScanPlan::scan treated an UnexpectedEof on the header or body read as the end of the data and returned Ok. Every byte below the captured end existed when the plan was taken, so a short read there can only mean the inode was truncated under the plan: an unmount followed by a VolumeCopy of the same volume id reopens .dat with truncate(true). The pass then reported Scanned, the next pass found the volume gone, and the stream ended cleanly after a prefix of the planned records, which volume.move would take as a complete tail. Both short-read arms now fail the scan with an I/O error that names the offset and the snapshot end, so tail_pass reports Status::internal as it does for every other read failure. The break arms were carried over from scan_raw_needles_from, where the whole scan ran under the store guard and nothing could truncate the file. Found by the Devin and Greptile reviews on #11275. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * rust volume: sum the needle padding in i64 so a corrupt size cannot overflow padding_length added the header, checksum and timestamp widths to the needle size in i32. A size read from a corrupt header can sit near i32::MAX, and that sum then overflows: a panic with overflow checks, a wrapped padding without. DatScanPlan::scan bounds the size against the bytes left before computing the body length, but that only keeps such a size out of the arithmetic while under 2 GiB of the file remains, so on a large volume the scan could still reach the overflow and, in release, size a buffer from garbage. Sum in i64 in both version branches. The result is at most NEEDLE_PADDING_SIZE, so it still fits Size. The scan comment no longer claims the bound check prevents the overflow. Found by the CodeRabbit review on #11275. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * rust volume: propagate dat scan parse failures --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
|
166af06a2b |
rust: cargo fmt both crates, with a commented-out fmt --check CI step (#11329)
* rust: migrate seaweed-volume and seaweed-worker to tonic 0.14 / prost 0.14
tonic 0.14 boxes the contents of tonic::Status, which is what made every
RPC path trip clippy's result_large_err; the allow for that lint goes in
the next commit. The prost codec moved out of tonic into tonic-prost and
tonic-prost-build, so both build scripts now call
tonic_prost_build::configure() and both crates depend on tonic-prost for
the generated code. The `tls` feature was split into a per-backend
feature; `tls-aws-lc` is the same backend both crates already install
through rustls::crypto::aws_lc_rs.
tonic 0.14 depends on axum 0.8 and tower 0.5, which would have left a
second axum and a second tower in each tree next to the 0.7 / 0.4 the
crates named themselves. Bumping them keeps one copy of each: axum 0.8
only changes the path-parameter syntax for the routes here (`/:vid` ->
`/{vid}`, `/*path` -> `/{*path}`), tower 0.5 needs the `util` feature
named explicitly for ServiceExt::oneshot (it used to arrive through
tonic's feature unification), and tower-http 0.6 is the matching
release.
Lock files move only through cargo's own resolution for the new
versions; no other dependency was refreshed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust: drop the result_large_err allow now that tonic::Status is boxed
tonic 0.14 stores Status behind a Box, so Result<_, Status> is no longer
a large-Err type and clippy has nothing to say about it. Both crates
pass `cargo clippy --all-targets -- -D warnings` without the allow
(seaweed-volume in both feature sets), so the policy entry and its
comment go.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: drop the unused headers argument of try_expand_chunk_manifest
The parameter was already named `_headers`; nothing in the body reads it.
With it gone the function is under clippy's argument threshold and the
expect goes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: pass EC peer reads an EcInterval instead of ten arguments
fetch_one_interval, read_remote_ec_shard_interval,
do_read_remote_ec_shard_interval and recover_one_remote_ec_shard_interval
all took the same (vid, needle_id, shard_id, shard_offset, size,
expected_encode_ts_ns) tuple, and the two that reconstruct also took the
location map with the data/parity counts. Those are now EcInterval (Copy)
and EcShardMap (a borrow of the map plus the counts). The fan-out inside
recovery builds its per-shard request with `EcInterval { shard_id: sid,
..iv }`, which is the one place the old argument list was easy to get
wrong. Bodies destructure at the top, so the code below the signatures
is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: give the EC encoder an EcEncodeLayout and an EncodeRun
encode_dat_file took the Reed-Solomon shape and three block sizes as five
loose integers; they are now one Copy struct, EcEncodeLayout, which is
what Go calls ECContext. The per-row and per-batch helpers took the same
six sinks and the offsets; they become methods on EncodeRun, which owns
the borrows for one run, so each call names only the offset and block
size that vary. The byte-level work is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: describe a .dat rebuild with DatRebuild instead of nine arguments
write_dat_file_from_shards, its _with_dirs twin and the private
write_dat_file were three layers over one nine-argument signature. One
public function now takes a DatRebuild, whose shard_dirs is None when
every shard sits beside the .dat and Some(dirs) for the cross-disk
reconciled layout. The field docs carry what the function doc used to
say about the encode-time size and the block layout.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: split copy_file_from_source's fifteen arguments into two structs
CopyFileSpec is the per-file request (what to ask the source for, where
it lands, whether its bytes count as progress); CopyProgress is the
sender, throttler and report state that all three files of one
VolumeCopy share, held by &mut across the calls. The three production
call sites now read as the .dat/.idx/.vif literals they are, instead of
positional trues and falses.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: create volumes from a VolumeSpec
Volume::new, DiskLocation::create_volume and Store::add_volume each
took the same five-value tail of Go's NewVolume argument list:
collection, replica placement, TTL, preallocation and needle version.
That tail is now VolumeSpec, a Copy struct whose Default is what almost
every test wanted anyway (empty collection, no replication, no TTL, no
preallocation, current version), so most of the 104 call sites shrink
to `&VolumeSpec::default()` or name the one field they set. The id,
directories, index kind and disk type stay positional because they
differ at every site.
Two imports that only test modules use moved into those modules, and
DiskLocation no longer imports ReplicaPlacement.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-worker: run cargo fmt
Layout only; no token in the workspace changes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: run cargo fmt
Layout only; no token in the crate changes. Every earlier Rust PR here
formatted only the blocks it touched so as not to drown its diff in
this one, and this commit is that debt paid in a single place. rustfmt
needed two passes to settle one block in handlers.rs; the committed
form is the fixed point, so `cargo fmt --check` is clean.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* ci: add a commented-out cargo fmt --check step to both Rust workflows
Same shape as the commented clippy step from #11312: the check is
written out so that making formatting a gate is a one-line uncomment,
and whether to do that stays a maintainer call.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
|
||
|
Eliah RusinandClaude Fable 5.1
|
517f60e875 |
rust-volume: fold the 8–15-argument functions into parameter structs (#11328)
* rust: migrate seaweed-volume and seaweed-worker to tonic 0.14 / prost 0.14
tonic 0.14 boxes the contents of tonic::Status, which is what made every
RPC path trip clippy's result_large_err; the allow for that lint goes in
the next commit. The prost codec moved out of tonic into tonic-prost and
tonic-prost-build, so both build scripts now call
tonic_prost_build::configure() and both crates depend on tonic-prost for
the generated code. The `tls` feature was split into a per-backend
feature; `tls-aws-lc` is the same backend both crates already install
through rustls::crypto::aws_lc_rs.
tonic 0.14 depends on axum 0.8 and tower 0.5, which would have left a
second axum and a second tower in each tree next to the 0.7 / 0.4 the
crates named themselves. Bumping them keeps one copy of each: axum 0.8
only changes the path-parameter syntax for the routes here (`/:vid` ->
`/{vid}`, `/*path` -> `/{*path}`), tower 0.5 needs the `util` feature
named explicitly for ServiceExt::oneshot (it used to arrive through
tonic's feature unification), and tower-http 0.6 is the matching
release.
Lock files move only through cargo's own resolution for the new
versions; no other dependency was refreshed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust: drop the result_large_err allow now that tonic::Status is boxed
tonic 0.14 stores Status behind a Box, so Result<_, Status> is no longer
a large-Err type and clippy has nothing to say about it. Both crates
pass `cargo clippy --all-targets -- -D warnings` without the allow
(seaweed-volume in both feature sets), so the policy entry and its
comment go.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: drop the unused headers argument of try_expand_chunk_manifest
The parameter was already named `_headers`; nothing in the body reads it.
With it gone the function is under clippy's argument threshold and the
expect goes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: pass EC peer reads an EcInterval instead of ten arguments
fetch_one_interval, read_remote_ec_shard_interval,
do_read_remote_ec_shard_interval and recover_one_remote_ec_shard_interval
all took the same (vid, needle_id, shard_id, shard_offset, size,
expected_encode_ts_ns) tuple, and the two that reconstruct also took the
location map with the data/parity counts. Those are now EcInterval (Copy)
and EcShardMap (a borrow of the map plus the counts). The fan-out inside
recovery builds its per-shard request with `EcInterval { shard_id: sid,
..iv }`, which is the one place the old argument list was easy to get
wrong. Bodies destructure at the top, so the code below the signatures
is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: give the EC encoder an EcEncodeLayout and an EncodeRun
encode_dat_file took the Reed-Solomon shape and three block sizes as five
loose integers; they are now one Copy struct, EcEncodeLayout, which is
what Go calls ECContext. The per-row and per-batch helpers took the same
six sinks and the offsets; they become methods on EncodeRun, which owns
the borrows for one run, so each call names only the offset and block
size that vary. The byte-level work is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: describe a .dat rebuild with DatRebuild instead of nine arguments
write_dat_file_from_shards, its _with_dirs twin and the private
write_dat_file were three layers over one nine-argument signature. One
public function now takes a DatRebuild, whose shard_dirs is None when
every shard sits beside the .dat and Some(dirs) for the cross-disk
reconciled layout. The field docs carry what the function doc used to
say about the encode-time size and the block layout.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: split copy_file_from_source's fifteen arguments into two structs
CopyFileSpec is the per-file request (what to ask the source for, where
it lands, whether its bytes count as progress); CopyProgress is the
sender, throttler and report state that all three files of one
VolumeCopy share, held by &mut across the calls. The three production
call sites now read as the .dat/.idx/.vif literals they are, instead of
positional trues and falses.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
* rust-volume: create volumes from a VolumeSpec
Volume::new, DiskLocation::create_volume and Store::add_volume each
took the same five-value tail of Go's NewVolume argument list:
collection, replica placement, TTL, preallocation and needle version.
That tail is now VolumeSpec, a Copy struct whose Default is what almost
every test wanted anyway (empty collection, no replication, no TTL, no
preallocation, current version), so most of the 104 call sites shrink
to `&VolumeSpec::default()` or name the one field they set. The id,
directories, index kind and disk type stay positional because they
differ at every site.
Two imports that only test modules use moved into those modules, and
DiskLocation no longer imports ReplicaPlacement.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
||
|
Eliah Rusin
|
49a680dd64 | rust: tonic 0.14 / prost 0.14, drop the result_large_err allow (#11327) | ||
|
|
adaf3534fa |
rust: clippy-clean both crates and adopt the std APIs the 1.91 MSRV allows (#11312)
* rust: apply clippy --fix to both crates The mechanical part of a clippy sweep: `cargo clippy --all-targets --fix` on seaweed-volume and the seaweed-worker workspace, hand-reviewed. Both manifests declare their MSRV (1.91.1 and 1.94.1), so every suggestion clippy applied is within it: the collapsible_if sites become let chains (1.88, edition 2024), `% n == 0` becomes is_multiple_of (1.87), chunks_exact with a constant becomes as_chunks (1.88), repeat().take() becomes repeat_n (1.82), and io::Error::new(Other, ..) becomes io::Error::other (1.74). The rest is redundant clones, borrows, casts, closures and field names. Nothing here changes behaviour. The three let_and_return sites in needle_map.rs and store_ec.rs deserve a note: the `let result = ..; result` shape was a deliberate edition-2021 workaround to drop a redb guard before the table it borrows. Edition 2024 drops tail-expression temporaries before locals, which is why clippy now flags it, and the two comments that described the workaround say so instead. Manual edits on top of the tool output: the blocks clippy rewrote are re-indented the way rustfmt lays them out (only those blocks — the crate is not rustfmt-clean and a whole-crate fmt would bury this diff), the blank lines let_and_return left behind are removed, and the CRC legacy_value test compares against a literal worked out from the original shift formula rather than restating rotate_right. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU * rust: clear the clippy warnings --fix cannot apply, and say why the rest stay Hand fixes for the lints clippy only reports. Behaviour is unchanged throughout; each rewrite is the one clippy names. - needless_range_loop (7): index loops over shard vectors become iterator loops. Where the old code indexed `v[..n]` the new loop iterates `v[..n]` so an undersized vector still panics the same way. - field_reassign_with_default (6): struct literals with `..Default`. - redundant_pattern_matching (3): `if let Err(_) = guard.check()` becomes `.is_err()`, which also releases the read guard at the end of the condition instead of at the end of the block. - manual_strip (2), manual_checked_ops, format_in_format_args, redundant_locals, wrong_self_convention (to_vif takes self by value, so it is into_vif; CompactEntry is Copy, so to_needle_value takes self). - type_complexity (2): `OrphanShardLoad` and `RawNeedleEntry` name two tuples that were spelled out inline. - new_without_default: CompactNeedleMap gets a Default that calls new(). - suspicious_open_options: a test helper spells out `.truncate(false)`, which is what `.create(true).write(true)` already did. What stays, and the attribute that says so: - too_many_arguments (10): `#[expect]` on each function. Folding 8–15 parameters into a struct is a design change, not a lint fix. - await_holding_lock / readonly_write_lock: one test holds the store write guard across a sleep on purpose, as a barrier that parks the copy task at the mount block. `#[expect(.., reason = ..)]` records it. - module_inception: needle/needle.rs mirrors the Go package layout. Two lints become crate-wide policy in `[lints.clippy]`, with the reason next to each: result_large_err, because every RPC path returns tonic::Status (176 bytes) and boxing it would change every handler signature; and needless_update, because `..Default::default()` on a protobuf message literal is what lets a proto gain a field without touching every constructor (all 11 sites are pb messages). The worker workspace gets the same table and its members opt in with `lints.workspace = true`; its generated plugin.rs also allows large_enum_variant on prost's oneof enums. Both crates are now clean under `cargo clippy --all-targets -- -D warnings`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU * rust volume: use the std APIs the 1.91 MSRV already pays for The crate declares rust-version 1.91.1, so a few things the code still worked around are plain std now. All of them come from the 1.85–1.91 release notes; nothing here needs a newer toolchain than the manifest already requires. - std::sync::LazyLock (1.80) replaces the lazy_static! block in metrics.rs, and the lazy_static dependency goes. Every use site reads the same through Deref, so no caller changes. - Duration::from_mins / from_hours (1.91) replace `from_secs(v * 60)` and `from_secs(v * 3600)` in the option parser and the shard-location refresh TTLs. One difference for the parser: an absurd count that overflows u64 seconds now panics in release builds too, where the multiplication used to wrap. - Result::flatten (1.89) replaces `.and_then(|r| r)` on the replication join handle. - OsStr::display (1.87) replaces `to_string_lossy()` where the name was only being formatted; the output is byte-identical. - `#[allow]` becomes `#[expect]` (1.81) on the suppressions that are meant to be permanent, so a suppression that stops being needed becomes a warning rather than lingering. Doing that found four that already had: dead_code on ChunkManifest, base_name and last_io_error, and too_many_arguments on read_from_data_shards, which is down to seven parameters. Those attributes are deleted. The three allows that depend on cfg (a unix-only mutation, a linux-only field set, a profiling-only parameter) stay as allow, because expect would be unfulfilled on the other platforms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU * ci: add a commented-out clippy step to both Rust workflows Both crates are warning-free under `cargo clippy --all-targets -D warnings` now. Whether that becomes a gate is a policy call, so the step is present but commented out; uncommenting it is the whole change. The comment points at the `[lints.clippy]` table where crate-wide exceptions are recorded, so the gate does not become a reason to sprinkle allows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjZY429aVU74SLDmo1wiuU * rust volume: guard parse_duration against overflow panics Duration::from_mins/from_hours panic when the count overflows u64 seconds. Use checked_mul so an oversized CLI value falls back to the parser default instead of crashing volume startup. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
15d9f6c6fe |
rust-volume: fix Windows build of find_needle_from_ecx (#11298)
* rust-volume: fix Windows build of find_needle_from_ecx The .ecx binary-search fallback path used on non-Unix targets (Seek + Read, both &mut self receivers) requires the ecx_file binding to be mutable. On Unix the read_exact_at path takes &self, so the mut would be unused there — gate that warning with #[cfg_attr(unix, allow(unused_mut))]. Without this the build-rust-volume-windows CI job fails with E0596 at ec_volume.rs:1033, breaking the weed-volume_windows_amd64 release asset. * rust-volume: use positional seek_read for .ecx lookups on Windows The previous fix (making ecx_file mut) compiled but left the Windows fallback using Seek + Read on the shared .ecx file cursor. Concurrent find_needle_from_ecx calls could interleave seek/read and read the wrong index entry, corrupting the binary search (raised by Devin and Greptile review on the PR). Switch the Windows path to std::os::windows::fs::FileExt::seek_read, which is positional (offset passed via OVERLAPPED, cursor untouched) and takes &self — so the binding no longer needs mut, and concurrent callers on the cached handle can't interfere. Mirrors the existing read_exact_at helper in storage::volume. Add a compile_error fallback for non-unix/non-windows targets to match the convention in storage::volume. |
||
|
|
5b2fe374fc |
[Volume] Scrub every disk's EC shards for a volume id, not just the first (#11258)
* storage: add Store::find_all_ec_volumes for split-disk EC lookups Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: add merge_ec_runtimes to resolve a vid's per-disk shard set Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: replace dead slots.get(14) assertion with a width-14 pin Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: build the checksum scrub plan from every per-disk runtime Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: build the local scrub plan from every per-disk runtime Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: prove the local scrub plan reaches every runtime's slots Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: make the scrub plan tests falsifiable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: report unverifiable protection when the sidecar predates the scrubbed encode Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: commit sidecar provenance with the sidecar it describes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: scrub every disk's EC shards for CHECKSUM and LOCAL Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: run the FULL/READS parity check across split-disk shards Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: report fenced-out runtimes in FULL/READS scrubs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: tighten verify_ec_shards ordering and missing-shard coverage Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: visit each EC volume id once in node-wide scrubs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: cover split-disk scrub aggregation end to end Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: pin fenced-out disks and sibling-disk shards in EC scrubs Three scrub behaviors shipped without a test at the RPC seam. Task 8 showed the seam exists, so close them here. FULL/READS (mode 2|5) now marks a volume broken when the identity fence excludes a runtime, where it previously reported clean. Pinned against a control fixture whose two disks AGREE and scrub clean, so the test fails on the clean->broken transition, not only on the message text. That needs a structurally valid, tombstone-only .ecx (so the needle walk finds nothing to complain about) and a seeded shard-location cache (so the absent master does not short-circuit the scrub with an error of its own). LOCAL (mode 3) and CHECKSUM (mode 4) now build their plans from every per-disk runtime. Made observable by moving shard 0 -- the shard the volume's single needle spans and the one the checksum sidecar is checked against -- to the SIBLING disk, leaving shard 5 on the disk the singular find_ec_volume lookup returns. Built from that disk alone, neither scrub ever looks at shard 0. The split-disk fixture grows a config struct rather than more positional arguments; its defaults reproduce the existing layout byte for byte, so the node-wide dedupe test is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: report fenced-out disks on a malformed sidecar too `errors.extend(self.skipped)` sat below the whole status match, so only `(Some(p), On)` ever reached it. The Invalid arm already returns a non-empty error vector of its own, so the Go-parity contract that silences the Off arm (`case BitrotOff: return 0, nil, nil`) does not reach it -- appending the fence lines there costs nothing that contract protects. A volume with BOTH a malformed sidecar and a disk the identity fence excluded reported only the sidecar, hiding the unscanned disk behind an unrelated integrity error. Off stays byte-identical, and so does the `(None, On)` arm that is documented as treating a missing payload defensively as protection off. Off is now the ONLY status that drops the report, and the comment at the On-path copy says so: that is the one place the parity constraint costs us coverage. Also corrects a false claim in the FULL/READS test's doc comment. It said a fenced-out disk "is a disk this scrub did NOT read", which is true only of the merge-driven parity half. The per-needle walk still resolves `store.find_ec_volume` (store_ec.rs:281) and binds `expected_encode_ts_ns` to that runtime (:311) -- position 0, the EXCLUDED one on that fixture -- so `read_local_intervals`' generation filter (:1204) makes it read the excluded disk and treat the anchor's shards as non-local, the inverse of what `skipped` reports. The fixture's tombstone-only .ecx walks nothing, so the test cannot tell the two apart; the comment now says that rather than implying coverage it does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: take CHECKSUM's bitrot protection from the disk that has the sidecar `EcChecksumScrubPlan::for_volumes` read `(prot, status)` off the ANCHOR. The anchor is the first shard-bearing runtime at the maximum `encode_ts_ns`, chosen with no regard for which disk holds the `.ecsum`. That sidecar is deliberately NOT mirrored across disks -- `ec_metadata_dirs()` exists so one authoritative copy stays reachable rather than being duplicated -- and at mount `EcVolume::new` resolves it via `load_active_bitrot_sidecar(&[])` with no sibling directories at all; only the `VolumeEcShardsMount` RPC ever passes `ec_metadata_dirs()`. So after EVERY volume-server restart, the split-disk runtime that does not physically hold the sidecar mounts `BitrotStatus::Off`. When the one copy lives on disk 1 and the anchor is disk 0, `run()` hit `case BitrotOff` and returned `(0, [], [])`: the whole volume scrubbed clean, silently. That is the steady state for roughly half of all mirrored split-disk layouts, and it is the exact failure this branch exists to remove. Source protection from the first MERGED runtime that has any -- `On` if one does, else `Invalid`, else the anchor's `Off`. Two facts make that safe, and both are load-bearing: - Every runtime that mounted `On` already passed the `geometry_matches` gate in `load_bitrot_for_generation`, so its manifest agrees with the volume's layout. A sidecar that contradicted it would have failed the mount. - All merged runtimes share the same `encode_ts_ns` by construction of the identity fence, so a sidecar from any of them describes the same encode run. The `unverifiable_sidecar` provenance rule four lines down read `anchor.bitrot_source_dir`; it now reads the SAME runtime `prot` came from. Otherwise the two would describe different sidecars and the rule would vouch for a manifest nobody is scanning against. One consequence worth naming: that source dir is now non-empty by construction (a runtime with protection found a file), where the anchor's was often "" and short-circuited the rule -- so on a fenced volume whose anchor had no sidecar, an unverifiable-protection note now surfaces where previously nothing was reported at all. `run()` is untouched, and the `BitrotStatus::Off` arm still returns `(0, [], [])` exactly, for Go parity with `case BitrotOff: return 0, nil, nil`. `parity_shards` still comes from the anchor while `prot` may come from a sibling; the geometry gate above makes them agree, and slot-width agreement is handled separately. The test drives mode 4 through the real RPC against a split-disk volume whose sidecar exists only on dir1, and asserts up front that the anchor mounted `Off` and the sibling `On` -- otherwise it would prove nothing. Reverting this commit's one-line source change makes it report `[]` instead of `[0, 5]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: pin the slot width, contain the shard-size fallback, and cover multi-disk FULL Five findings from the whole-branch review, none of which changes what a healthy volume reports. Slot width was undefined and the two consumers disagreed (ec_volume.rs). `merge_ec_runtimes` sizes `slots` to the WIDEST merged runtime, but the identity fence keys on `encode_ts_ns` alone and never on geometry -- so two same-generation runtimes whose `.vif`s disagree do merge. The mode 2|5 arm truncates to the anchor's `data+parity` and silently drops the surplus slots, while `EcChecksumScrubPlan::for_volumes` iterated the full width and emitted "present but missing from sidecar manifest" for exactly those ids. Nothing in the volume describes them -- the sidecar manifest and the Reed-Solomon matrix are both the anchor's -- so that message was the width disagreement talking, not a finding. The `slots` field doc now states the contract (the range is the anchor's geometry; every consumer truncates to it) and CHECKSUM truncates. The LOCAL `shard_size` fallback had grown a node-wide blast radius (ec_volume.rs). `anchor.shard_file_size()` returns the anchor's FIRST held shard, not a maximum. Before aggregation the plan read only that runtime's own shards, so a truncated shard was contained to its disk; now that one value sizes every merged sibling's shards, mis-offsetting `locate_data` and manufacturing needle corruption across the node. Take the max over the merged slots, which is how `verify_ec_shards` already answers the same question (`if size > shard_size { shard_size = size }`). Only on the legacy `dat_file_size == 0` path. Multi-disk `all_local` had no end-to-end test (grpc_server.rs). The parity check is gated on every shard being present, and the one all-local fixture keeps them in a single directory, so every entry of `dirs` is the same string and a permutation or off-by-one in the `slots` -> `dirs` mapping is invisible; `test_verify_ec_shards_reads_shards_from_multiple_dirs` builds its `dirs` by hand and never goes through `merge_ec_runtimes`. The new fixture is a real 10+4 encode split 0..=6 / 7..=13 across two store locations (the `.dat`/`.idx` stay outside both, so `prune_incomplete_ec_with_sibling_dat` has nothing to act on), driven through the real RPC: clean first, then a corrupted PARITY shard on the SECOND disk -- which only the parity half can see, and only through a correct mapping. Shifting that mapping by one, or computing `all_local` from the anchor alone, both make it report `[]` instead of `[13]`. Deleted `test_ec_volume_enumeration_is_deduped` (store_ec_reconcile.rs). It built `raw` from `store.locations` and then applied its OWN inline `filter(|v| seen.insert(*v))`, asserting on that -- a property of `HashSet::insert`, never reaching the production dedupe. That path is covered by `test_scrub_ec_volume_node_wide_dedupes_a_split_disk_volume`, which does fail (2 != 1) when the dedupe is removed. Corrected `test_verify_ec_shards_treats_a_none_dir_as_missing`'s docstring (ec_encoder.rs). It claimed the unmounted shard "must not drag the shards that ARE mounted down with it", but `dirs[5] = None` puts shard 5 in `broken_shards` before the block loop, so every iteration takes the `read_failed` arm and the parity comparison never runs: corrupting a mounted shard in that fixture changes nothing about the result. The assertions are unchanged; the docstring now states what they actually establish. Also refreshed two comments that cited `shard_file_size() - 1` as the reason `merge_ec_runtimes` prefers a shard-bearing anchor -- true before this commit, stale after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: correct the Fix 1 rationale and truncate the shard-size scan The safety argument attached to `EcChecksumScrubPlan::for_volumes`'s protection selection was false as written, and it is the argument a reviewer reads first. `geometry_matches` compares a sidecar against the MOUNTING runtime's own data/parity/block size, not the anchor's, and returns true vacuously when `ec_shard_config` is `None` -- so it establishes agreement only when all merged runtimes share one geometry, which an `encode_ts_ns`-only fence does not guarantee and which `test_checksum_scrub_truncates_slots_to_the_anchors_geometry` constructs a counterexample to. The second clause was weaker than stated too: a `.ecsum` records no encode identity at all, so merged runtimes agreeing on `encode_ts_ns` does not transfer to the sidecar. Replace it with the property that is true, checkable from the selection itself, and stronger for what actually matters. `anchor` is an element of `merged`, so the `.unwrap_or(anchor)` fallback is reached only when no merged runtime is `On` and none is `Invalid` -- in which case the anchor is necessarily `Off`. The status can therefore only move `Off -> On`, `Off -> Invalid` or `Invalid -> On`; never `On -> Off`, never `Invalid -> Off`. This selection cannot stop a volume that was being scanned from being scanned, and cannot turn a reported integrity error into silence: every change it makes is toward more verification. The comment now also states what it does NOT establish -- geometry agreement is not guaranteed -- and names geometry fencing as the follow-up that would close it. Second, `EcLocalScrubPlan::for_volumes`'s `shard_size` max scanned the FULL slot width, violating the `slots` contract documented in the same commit that introduced the max: the volume's shard-id range is the anchor's geometry and every consumer must truncate to it. Pre-fix that input could not exist, because `anchor.shard_file_size()` read only the anchor's own anchor-sized vector -- so the max opened a new, narrow path to the same node-wide mis-sizing it exists to close (same-generation runtimes with disagreeing `.vif`s, the wider one holding an out-of-geometry shard larger than the in-geometry ones, `dat_file_size == 0`). `.take(anchor.data_shards + anchor.parity_shards)` mirrors the truncation already applied to the CHECKSUM shard scan. The sibling `shards:` vector is left untruncated on purpose: every access in `EcLocalScrubPlan::run` is `shards.get(sid)` with `sid < data_shards`, so the surplus entries are inert. No behavior change for any healthy volume, and no test added -- the suite is unchanged at 575 passing, 0 failing, 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: aggregate split-disk runtimes in Go scrubs, mirroring Rust Go volume scrubs previously used FindEcVolume (first runtime only), so a volume whose EC shards are split across multiple disks was scrubbed against just one disk's shards and the others were silently skipped. Node-wide ScrubEcVolume also appended each disk's EcVolumeIds without deduplication, scrubbing a split-disk volume once per disk. Add MergedEcRuntimes/MergeEcRuntimes (Go counterpart to Rust's merge_ec_runtimes): select the maximum EncodeTsNs as the anchor generation, fence out runtimes whose encode generation or geometry (DataShards, ParityShards, BlockSize) disagrees with the anchor, merge shard handles by shard ID, and report excluded runtimes rather than dropping them. Wire it into every scrub mode: - INDEX: scrub the anchor's index, report skipped runtimes. - LOCAL: aggregate local shards across all merged runtimes via a synthetic EcVolume built from the merged shard slots. - FULL/READS: resolve the runtime matching the anchor's encode generation (not the first match) so the needle walk and parity phase inspect one encode run; report skipped runtimes. - CHECKSUM: take bitrot protection from the first merged runtime that has a valid sidecar (On, else Invalid, else anchor's Off), preserve invalid sidecar errors from every other merged runtime, and report skipped runtimes. Deduplicate EC volume IDs in node-wide ScrubEcVolume so each volume is scrubbed exactly once. Refactor ScrubEcVolume to share the per-needle walk via scrubEcVolumeWalk, called by both the legacy first-runtime path and the new merged path. Add Go regression tests covering split-disk deduplication, encode-generation fencing, geometry fencing, sibling-disk LOCAL reach, and merge anchor selection. Rust: keep the previously-landed merge/fence/checksum changes intact; revert incidental cargo-fmt drift from unrelated files so the diff stays focused. * ec: fence merged CHECKSUM on sidecar encode generation and fix legacy shard size Address two review findings on the Go merged-runtime scrub: 1. Sidecar provenance: a merged runtime can load a bitrot sidecar from a sibling metadata directory (ReloadBitrotSidecar), and the merge fence may then exclude the runtime owning that directory. Generation-0 sidecars do not identify the encode run, so geometry validation alone cannot prove the borrowed manifest describes the anchor shards. If the sidecar records a non-zero EncodeTsNs that disagrees with the anchor, refuse the scan instead of applying stale checksums to current shards and reporting false corruption. 2. Legacy shard size: for volumes without datFileSize in .vif, LocateEcShardNeedleInterval derives the shard size from Shards[0].ecdFileSize. The merged shard set is compacted in shard-ID order, so a truncated lowest-ID shard would shrink every interval and misread intact sibling shards. Synthesize a datFileSize from the maximum mounted shard size when the anchor lacks one, so the datFileSize>0 path uses the largest shard size across all merged runtimes. * ec: fix copylocks, legacy shard boundary, and encode-aware Rust lookups Address review findings from CodeRabbit and Devin: Go (ec_volume_merge.go): - Remove bitrotLock copy from the synthetic EcVolume: copying a sync.RWMutex is a go vet copylocks error. The synthetic volume uses its own zero-value mutex; bitrot/bitrotStatus are set directly before ChecksumScrub reads them via BitrotProtection(), so no concurrent access occurs. - Fix legacy shard-size boundary: synthesize datFileSize from (maxShardSize - 1) * DataShards, not maxShardSize * DataShards, to match the legacy fallback in LocateEcShardNeedleInterval (ecdFileSize - 1). An exact large-block boundary is ambiguous; the unadjusted size would select an extra large row and misread intact sibling shards. Rust (store_ec.rs): - Add find_ec_volume_for_scrub helper that resolves by encode generation (not first-match find_ec_volume) and use it in scrub_snapshot_under_lock, write_back_shard_locations, and the post-refresh shard-location read. Previously the encode-aware lookup was only used for the initial runtime selection; the cache write-back and per-needle snapshot still used first-match, so a split-disk volume whose first runtime was from an older encode run would write to and read from the wrong runtime's shard-location cache and falsely abort with 'remounted as a different encode run'. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Da.Sanchezandchrislusf
|
d8aa7ecf04 |
fix(vacuum): stop comparing compact size against the live needle map (#11263)
* fix(vacuum): stop comparing compact size against the live needle map CompactByIndex's post-copy integrity check compared bytes written to the .cpd against v.nm.ContentSize()-DeletedSize(), the live map that keeps mutating for as long as the volume stays writable during the copy. Any write landing after the point-in-time index snapshot was loaded made the live map's tally exceed what got copied, aborting compaction with "unexpected new data size" — even though CommitCompact's makeupDiff exists specifically to reconcile writes that land mid-copy. On a busy volume this can fail every vacuum cycle. Tally the expected live size from oldNm, the same frozen snapshot the copy loop reads from, instead of the live map. This keeps the check's original protection (destination smaller than what should have been copied signals real data loss) while removing the false positive from ordinary concurrent traffic. * fix(vacuum): stop double-subtracting skipped bytes from the size check Unreadable needles return before reaching the expectedLiveBytes tally, so it already excludes them. Subtracting skippedDataBytes again on top loosened the integrity check's margin by that same amount, letting a .cpd short of the true expected size slip past undetected — the exact failure mode the check exists to catch. Flagged independently by three automated PR reviewers (Devin, Greptile, CodeRabbit). Extract the comparison into exceedsExpectedCompactedSize and drop the subtraction entirely; add TestExceedsExpectedCompactedSize to pin the threshold to expectedLiveBytes alone. * fix(vacuum): trim verbose integrity-check comment Reduce the 8-line block comment to a concise 3-line rationale. No behavior change. * fix(vacuum): mirror compact integrity check in Rust volume server Mirror the Go fix in the Rust volume server's do_compact_by_index: tally expected_live_bytes from the frozen index snapshot (not the live needle map) and compare the compacted .dat against it after the copy. Unreadable needles already return before the tally, so no skipped-byte adjustment is needed. Adds exceeds_expected_compacted_size and two regression tests. * fix(vacuum): exercise makeup_diff in Rust concurrent-write test Address CodeRabbit review: write a needle after compaction (before commit), then call commit_compact() and assert the late write survives via makeup_diff. This actually exercises the concurrent-write path rather than just confirming the integrity check passes. --------- Co-authored-by: chrislusf <chris.lu@gmail.com> |
||
|
|
3ae9e332ec |
rust volume: honour is_last in the tail sender instead of rescanning the whole volume (#11273)
* rust volume: honour is_last in the tail sender instead of rescanning
volume_tail_sender discarded the is_last flag from
binary_search_by_append_at_ns:
Ok((offset, _is_last)) => {
if offset.is_zero() { Ok(sb_size) } ...
is_last means the caller is already caught up. Go answers that with a
heartbeat and does not scan at all (volume_grpc_tail.go, `if isLastOne`).
Dropping it is expensive rather than untidy, because the branches interact:
when the search reports caught-up it returns Offset::default(), which is
zero, so the start offset falls back to sb_size -- the beginning of the
data -- and scan_raw_needles_from materialises every needle from there to
EOF into a Vec. The timestamp filter discards all of it, the loop sleeps
2s, and it happens again.
A volume being moved is marked read-only before the copy, so it is ALWAYS
caught up during the tail phase. Measured on one volume.move of a 2.15 GB
volume, sampling the source's cgroup anon every 2s against the move's own
phase output:
copying 16 -> 37 MB CopyFile streams correctly, stays bounded
tailing 904 -> 2166 -> 629 -> 2166 -> 342 -> 2173 -> 2179 MB
deleting 46 MB
Six full-volume allocate/free cycles in 35s, peak 2179 MB against a volume
of 2147 MiB. The destination never exceeded 35 MB, so this is entirely
source-side. Under a per-process memory cap it OOM-kills the source
whenever the volume exceeds the cap.
The ordering here is the whole fix and is easy to get wrong: resolve the
start offset and is_last under a brief lock, return the heartbeat
immediately when caught up, and only then reach the scan. An earlier cut
set the flag correctly but placed the early return after the block that
performs the scan -- the heartbeat fired and the destination received
nothing, yet every iteration still read the whole volume and discarded it.
Production showed no improvement (1770 MB across five cycles), which is
what caught it. The binary search is over the .idx and costs nothing; the
scan is the expensive part and must not run speculatively.
Three tests, and the last two matter as much as the first: a fix that
always reported "caught up" would make tailing silently lose needles, a
worse bug than the one being fixed. One asserts is_last for a caller at or
beyond the newest append_at_ns; one asserts NOT is_last for a caller that
is behind, so real tail data is still scanned and shipped; one asserts NOT
is_last when the only newer record is a delete, and that scanning from the
returned offset ships exactly that tombstone.
Left deliberately unfixed, and worth separate changes: the scan still
collects into a Vec rather than streaming through a visitor as Go's
ScanVolumeFileFrom does, and it runs while holding store.read(), the same
lock-across-a-large-read shape as #11235. Both are latent once the rescan
is gone, since remaining scans are bounded by genuinely new data.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MFr2v4BUqrXdgj4LEUAwVF
Claude-Session: https://claude.ai/code/session_018VF7E9SHPihG1jC1grU9H3
* rust volume: resolve and scan the tail under one store guard
The tail sender took store.read() once for the binary search and again
for the scan. A vacuum commit takes the store write lock and swaps
.dat/.idx, so it could land between the two: the offset resolved against
the old files would then be applied to the new ones and start the scan
inside an unrelated record. The code before the is_last fix held a
single guard for both. Restore that, and scan only when the caller is not
caught up, so the caught-up heartbeat still skips the scan and is sent
outside the lock.
Also pin the compacted-volume boundary raised in review. Compaction
writes .idx in needle-id order in both Go and Rust, so the search can
report caught-up while an earlier row is newer; such a caller's since_ns
is the last row's timestamp, so those rows were in the files it copied.
A write made afterwards is appended as the final row, which the search
cannot step past. The new test asserts it still reaches the scan.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VF7E9SHPihG1jC1grU9H3
* rust volume: make the compaction tail test a genuine overwrite
The compaction regression test's second id=1 write reused the first
write's data, so write_needle's dedup short-circuit (is_file_unchanged)
returned without appending or updating append_at_ns. Compaction then
kept key 1's original (older) timestamp, so the test passed without
exercising the overwrite it describes -- key 2 was the final row only
because key 1 was never actually newer.
Give the overwrite distinct data so it appends a new record, and assert
key1_ns > key2_ns up front so a future dedup regression fails the test
instead of silently hollowing it out. Trim the verbose comments on the
tail sender and the binary-search tests to their essentials.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
|
4f9bbd51cb |
rust volume: stop glibc retaining freed EC buffers as unreturnable heap (#11255)
* rust volume: stop glibc retaining freed EC buffers as unreturnable heap
A Rust volume server doing EC work accumulates hundreds of MB of resident
anonymous memory that it never gives back, and under a hard cgroup
MemoryMax that ends in an OOM kill while most of the resident set is
free-but-unreturned.
It is not a leak. glibc serves allocations >= M_MMAP_THRESHOLD with mmap
and munmaps them on free, but the threshold is ADAPTIVE: freeing an
mmap'd block raises it toward that block's size, up to 32 MiB. EC
reconstruction and needle reassembly allocate large short-lived buffers,
so the first few train the threshold upward and every later buffer is
carved from the heap instead. Heap pages only return to the OS from the
top of the arena, so they stay resident for the life of the process --
reusable, but anonymous, and anonymous pages cannot be reclaimed under
pressure the way page cache can. The retained footprint is exactly the
headroom a burst of maintenance work needs.
Measured on a 17-node cluster (EC 10+4, --index=redb), one node, two
identical `ec.scrub -mode full` rounds over 10912 EC files each, same
unit restarted with and without a pinned threshold:
baseline round 1 round 2 60s idle
default (adaptive) 10 MB 84 MB 88 MB 88 MB
pinned threshold 10 MB 13 MB 14 MB 14 MB
78 MB retained versus 4 MB for identical work. On heavier mixed scrub
workloads the same effect reached ~600 MB per volume server against a
3 GiB cap, and restarting the process was the only way to release it.
Calling mallopt(M_MMAP_THRESHOLD, ...) sets the threshold and disables
the dynamic adjustment. Pin it to glibc's own default rather than
inventing a value: the goal is to stop the adaptation, not to second-guess
the default. MALLOC_MMAP_THRESHOLD_ still wins if an operator sets it,
glibc-only, and a failed mallopt is logged rather than fatal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MFr2v4BUqrXdgj4LEUAwVF
* Address PR review: validate env overrides, honour GLIBC_TUNABLES, fix non-glibc test compile
Three review-bot findings on seaweed-volume/src/malloc_tuning.rs:
1. (CodeRabbit) The test used cfg!(...), which keeps both branches in
compilation. On non-glibc targets DEFAULT_MMAP_THRESHOLD is undefined,
so the test failed to compile. Split into #[cfg]-gated tests so each
branch only references items defined for that target.
2. (Greptile) MALLOC_MMAP_THRESHOLD_ was checked by presence only. An
empty or non-numeric value makes glibc ignore the override while we still
skipped mallopt, leaving the adaptive threshold enabled -- exactly the
behaviour this module exists to prevent. Now we defer only when the value
is non-empty and parses as an integer; otherwise we fall through to
pinning.
3. (Codex) The modern GLIBC_TUNABLES=glibc.malloc.mmap_threshold=... tunable
was missed, so mallopt could overwrite an operator's explicit tunable. Now
we detect that tunable (with the same validation) and defer to it.
The override check moved into the glibc-gated inner function, so off glibc
pin_mmap_threshold() always reports NotApplicable regardless of any
allocator env vars that happen to be set. The startup log for DeferredToEnv
is reworded to cover both override sources. Added tests for the override
parsers and the off-glibc no-op.
* Address round-2 review: match glibc's actual override parsing
Three follow-up review-bot findings after the first round of fixes, all
rooted in our validation not matching how glibc actually parses the
overrides:
1. (Greptile, P1) parse::<i64>() accepted negative values like "-1" and
returned DeferredToEnv, but glibc's threshold is unsigned and rejects
negatives — so we skipped mallopt while glibc also ignored the override,
leaving the adaptive threshold enabled. Now we reject negatives and
zero.
2. (Devin, BUG) glibc parses thresholds as unsigned (strtoul for tunables,
atoi for the legacy var). Values above i64::MAX are valid for glibc but
were rejected by parse::<i64>(), so we pinned 128 KiB over the operator's
explicit setting. Now we parse as u64, accepting the full unsigned range.
3. (CodeRabbit, Major) Two issues in usable_glibc_tunable_threshold:
a. A malformed sibling entry (e.g. glibc.malloc.check=2=2:...) makes
glibc reject the entire GLIBC_TUNABLES string, but our per-entry scan
still returned true for the valid-looking mmap_threshold entry. Now
we validate every entry (exactly one '=') before accepting any.
b. Hex values (0x20000) are accepted by glibc's strtoul but were rejected
by parse::<i64>(). Now parse_strtoul_threshold handles 0x-prefixed hex.
MALLOC_MMAP_THRESHOLD_ stays decimal-only (atoi), matching glibc.
Added regression tests for negatives, zero, >i64::MAX, hex tunables, and
malformed mixed GLIBC_TUNABLES entries. Verified: clippy clean and tests
pass on macOS (non-glibc); glibc-gated code type-checks for
x86_64-unknown-linux-gnu.
* Address round-3 review: match glibc's actual override parsing
Three follow-up review-bot findings (Greptile P1, Devin BUG, CodeRabbit
Major) all on the same issue: the round-2 fix rejected negative and zero
override values, but glibc actually accepts them.
Verified against the glibc source (malloc/malloc.c, malloc/arena.c,
elf/dl-tunables.c, elf/dl-misc.c):
- do_set_mmap_threshold(size_t value) does NO clamping — it just sets
mp_.mmap_threshold = value and mp_.no_dyn_threshold = 1.
- MALLOC_MMAP_THRESHOLD_: glibc calls atoi(value) then mallopt, which
always sets the threshold and disables dynamic adjustment — even for
empty, negative, or non-numeric values (atoi returns 0). So ANY
presence of the variable means the operator's override is in effect.
Reverted to presence-only check for the legacy variable. The round-1
Greptile comment claiming glibc "cannot apply the override" for
empty/malformed values was incorrect.
- GLIBC_TUNABLES: glibc parses values with _dl_strtoul (elf/dl-misc.c),
which accepts decimal, 0x hex, 0 octal, an optional sign (negatives
wrap to unsigned long), and requires the entire value consumed
(tunable_parse_num checks endptr == strval + len). Replaced
parse_strtoul_threshold with dl_strtoul_consumes_all that replicates
_dl_strtoul's parsing and checks full consumption. Now accepts -1
(wraps to SIZE_MAX), 0, 0x20000, 010 (octal), and values above
i64::MAX.
The duplicate-= validation for GLIBC_TUNABLES (from round 1) is kept —
glibc's parse_tunables_string returns -1 if any entry's value contains
a duplicate =, rejecting the entire string.
Added dl_strtoul_consumes_all tests covering decimal, hex, octal,
negative, zero, empty, whitespace, trailing garbage, and sign-only
inputs. Updated usable_glibc_tunable_threshold tests to accept
negative, zero, and empty values. Verified: clippy clean and tests
pass on macOS (non-glibc); glibc-gated code type-checks and clippy
clean for x86_64-unknown-linux-gnu.
* Address round-4 review: add overflow detection, fix sign-only test assertions
Two Greptile P1 findings:
1. Overflowing tunables bypass threshold pinning: dl_strtoul_consumes_all
consumed every digit and returned true for values like
18446744073709551616 (u64::MAX + 1), but glibc's _dl_strtoul stops at
the overflowing digit (sets endptr there, returns UINT64_MAX), so
tunable_parse_num rejects the value (endptr != strval + len). Added
overflow detection matching glibc's cutoff/cutlim logic — on overflow,
the parser stops and returns false.
2. Sign-only parser assertions fail: the test asserted
!dl_strtoul_consumes_all("-") and !dl_strtoul_consumes_all("+"), but
_dl_strtoul skips the sign, finds no digit, sets endptr to the position
after the sign (== end of string), and returns 0. tunable_parse_num
sees endptr == strval + len → true. So glibc accepts sign-only strings
as value 0. Fixed the test assertions to expect true.
Also fixed "0x" with no hex digits: _dl_strtoul parses "0" as octal, then
stops at "x" (not an octal digit), so endptr != end of string → rejected.
The base-detection now requires a hex digit after "0x" before switching
to hex; otherwise "0" is parsed as octal and "x" stops the parser.
Added overflow regression tests: 18446744073709551616 (u64::MAX + 1),
99999999999999999999 (20 nines), 0x10000000000000000 (2^64). Verified:
clippy clean and tests pass on macOS (non-glibc); glibc-gated code
type-checks and clippy clean for x86_64-unknown-linux-gnu.
* Address round-5 review: accept bare 0x prefix, remove unused helper
Two review-bot findings (Devin BUG + CodeRabbit Major) on the same issue:
the round-4 fix required a hex digit after "0x" before switching to hex
base, but glibc's _dl_strtoul unconditionally advances past "0x"/"0X"
when the first char is '0' and the next is 'x'/'X' — even if no hex digit
follows. In that case the digit loop breaks immediately, endptr reaches
the end, and the value is 0. tunable_parse_num accepts it.
Removed the is_digit_in_base lookahead from the base-detection condition
and the now-unused is_digit_in_base helper. Updated the test assertions
for "0x" and "0X" to expect true (accepted as value 0).
The Greptile P1 overflow comment is invalid: glibc's _dl_strtoul rejects
18446744073709551616 (u64::MAX + 1) — on overflow it sets endptr to the
overflowing digit (not end of string) and returns UINT64_MAX, so
tunable_parse_num sees endptr != strval + len and rejects. My
implementation correctly returns false for this value, matching glibc.
Verified: clippy clean and tests pass on macOS (non-glibc); glibc-gated
code type-checks and clippy clean for x86_64-unknown-linux-gnu.
* Address round-6 review: rewrite tunable parser to match glibc exactly
Two Greptile P1 comments (3975151906, 3975151911) both invalid, but
investigation revealed a real bug in the split(':')-based parser:
Bug: usable_glibc_tunable_threshold used split(':') which loses the
distinction between an entry terminated by ':' (glibc skips it) and one
terminated by '\0' with no '=' (glibc rejects the entire string). Examples:
- "glibc.malloc.mmap_threshold=262144:glibc.cpu.x" (no '=' at end):
glibc rejects entire string, old code accepted it.
- "glibc.malloc.mmap_threshold=262144:" (trailing ':'):
glibc rejects entire string, old code accepted it.
Fix: replaced split(':') with a character-by-character parser matching
glibc's parse_tunables_string exactly. The parser tracks position in the
original string and correctly handles all three terminators ('=', ':', '\0')
for both name and value scanning.
Comment 3975151906 (near-maximum values): Invalid. Verified against
_dl_strtoul: for 18446744073709551615 (u64::MAX), cutoff = u64::MAX/10,
cutlim = u64::MAX%10 = 5. After 19 digits result == cutoff. 20th digit 5:
overflow check (digval > cutlim) is 5 > 5 = false → no overflow. glibc
accepts u64::MAX. Added regression test asserting it's accepted.
Comment 3975151911 (later malformed entry): Invalid. Verified against
parse_tunables (elf/dl-tunables.c): when parse_tunables_string returns -1,
parse_tunables prints a warning and returns immediately without applying
ANY tunable — including ones already parsed into the array. Added
regression test for "threshold=262144:check=2=2" (threshold before
malformed sibling) asserting it's rejected.
Added regression tests: u64::MAX accepted, threshold-before-malformed
rejected, no-'=' at end rejected, trailing ':' rejected, leading ':'
accepted. Verified: clippy clean and tests pass on macOS; glibc-gated
code type-checks and clippy clean for x86_64-unknown-linux-gnu.
* Fix CI: correct hex trailing-garbage test assertion
The test asserted !dl_strtoul_consumes_all("0x20000abc"), but in hex
mode a-f are valid digits — "0x20000abc" is a valid hex number
(0x20000abc = 536874044), not trailing garbage. _dl_strtoul consumes
the entire string and tunable_parse_num accepts it. The assertion
failed on Linux CI where the glibc-gated test actually runs.
Replaced with "0x20000g" — 'g' is not a hex digit, so _dl_strtoul
stops at 'g' and tunable_parse_num rejects the value.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
Chris Lu
|
3c9a4bbdda |
rust: prevent phantom volumes + validate collection hint in mount_volume_by_id (#11254)
* rust: prevent phantom volumes + validate collection hint in mount_volume_by_id
The collection-hint path (and the find_volume_file_base fallback) called
create_volume on any matching .vif/.idx sidecar. create_volume ->
Volume::new -> load(create_dat_if_missing=true) writes an empty .dat and
registers a phantom normal volume, which can shadow a real EC volume whose
.ecx lives on a sibling disk. This reintroduces the phantom-volume bug the
codebase explicitly guards against in load_existing_volumes.
Apply the same guard load_existing_volumes uses to both paths: only mount
when a real .dat is present or the .vif references a remote-tiered file;
otherwise skip the candidate (no phantom). Also reject path-bearing
collection hints ('/', '\\', '..') so the shortcut cannot route .dat
creation outside the storage directory, falling back to the safe scan.
Adds 3 regression tests; all 336 storage:: tests pass.
Addresses Devin + Greptile review comments on PR #11249.
* rust: address review — .note guard, multi-candidate scan, foo..bar hint
Address the four review comments on #11254:
1. Greptile (P1): contains("..") rejected valid collections like "foo..bar".
Replaced with collection != ".." — volume_file_name joins with "_" so a
".." inside a name is part of the filename, not a parent reference. Only
the exact ".." name is rejected. Added a test that "foo..bar" mounts.
2. Devin #0001 (bug): mount_volume_by_id did not check the .note marker, so
an interrupted VolumeCopy could mount as a live (truncated) volume. Added
a .note check before create_volume in both the collection-hint path and
the fallback — a candidate with .note is skipped (matches
load_existing_volumes). Added a test covering both paths.
3. Devin #0002 + CodeRabbit (major): find_volume_file_base returned only the
first matching candidate, so a lone sidecar on disk 0 hid a real .dat on
disk 1 (the split-disk EC layout the phantom guard protects against).
Added find_volume_file_bases (plural) that collects all candidates; the
fallback now iterates every candidate and mounts the first with a real
.dat or remote .vif. find_volume_file_base delegates to it for
configure_volume. Added a two-disk test: sidecar on disk 0, real .dat on
disk 1 — mount succeeds from disk 1.
All 339 storage:: tests pass (6 mount_volume_by_id tests).
* rust: continue past create_volume failure in mount_volume_by_id
Address Devin review comment on #11254: when create_volume fails on an
earlier candidate (e.g. an unreadable .dat), mount_volume_by_id returned
the error immediately instead of trying later candidates. A valid volume
on another disk remained unmounted.
Both the collection-hint loop and the find_volume_file_bases fallback now
remember the last error and continue scanning. A successful mount returns
immediately; if no candidate succeeds, the last error (or NotFound) is
returned. Matches DiskLocation::open_volumes and Go Store.mountVolume.
Added test_mount_volume_by_id_continues_past_open_failure (chmod 000 .dat
on disk 0, real volume on disk 1, mounts from disk 1).
All 340 storage:: tests pass.
|