mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-30 03:35:37 +00:00
83d44be0f30f7b2cffb6bfdef8041b4bd8bc75c2
209
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
      Eliah RusinGitHubClaude Fable 5.1Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>franchbChris LuDevin <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>
|
||
|
Eliah RusinGitHubClaude Fable 5.1Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>Chris LuDevin <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>
|
||
|
|
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> |
||
|
|
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> |
||
 
|
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 LuandGitHub
|
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>
|
||
|
|
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> |
||
|
Eliah RusinGitHubDevin <158243242+devin-ai-integration[bot]@users.noreply.github.com>Claude Fable 5.1Chris LuDevin <158243242+devin-ai-integration[bot]@users.noreply.github.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> |
||
|
|
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> |
||
|
Eliah RusinGitHubDevin <158243242+devin-ai-integration[bot]@users.noreply.github.com>Claude Fable 5.1Chris LuDevin <158243242+devin-ai-integration[bot]@users.noreply.github.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> |
||
|
|
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> |
||
|
|
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> |
||
 hsdfatandGitHub
|
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. |
||
|
|
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> |
||
|
|
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 LuandGitHub
|
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 LuandGitHub
|
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. |
||
|
|
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>
|
||
|
|
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 RusinandGitHub
|
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 RusinandGitHub
|
def25ca84d | fix(ec): validate ShardId at gRPC boundary, reject >=32 (#11346) | ||
|
Eliah RusinandGitHub
|
701e397337 | fix(volume): reject negative Size, recover poisoned store lock (#11345) | ||
|
Eliah RusinandGitHub
|
4fc9ada2ec | ci: run seaweed-volume unit tests on Windows (#11349) | ||
|
|
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>
|
||
|
|
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 RusinandGitHub
|
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 LuandGitHub
|
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> |
||
|
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 LuandGitHub
|
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.
|
||
 Feng ShaoandGitHub
|
13bf056a15 |
Mount req with collection (#11249)
* volume mount req support specify collection * rust mirror change |
||
|
|
516e251f9e |
rust volume: move the crate to edition 2024 (#11244)
* rust volume: move the crate to edition 2024 Edition 2024 turns three things in this crate into hard errors, and changes drop order in a further 34 places without changing compilation. The compiler errors are fixed here; the silent changes were audited against `RUSTFLAGS='-W rust-2024-compatibility' cargo check --all-targets` output captured before the flip, since edition 2024 stops reporting them. `std::env::set_var`/`remove_var` are unsafe as of 2024 because they race with concurrent readers. All six call sites are safe by construction rather than by assertion, and the SAFETY comments say why: the build script runs single-threaded before anything else in the process, and every test reaching the `config.rs` helpers holds `process_state_lock()` for the duration. The two `ref` bindings in handlers.rs sit in patterns that already borrow implicitly, so removing the modifier leaves both bindings at `&String`. On the 34 drop-order sites: no lock guard's scope is extended anywhere, and `volume.rs` has none. Most are moved-from `Option`/`Result` husks — `if let Some(v) = map.remove(&k)`, `while let Some(m) = stream.next().await` — where the value is moved into the binding and the temporary has nothing left to drop; where closing order actually matters these paths already call `v.close()`, `ec_vol.destroy()` or `drop(writer)` explicitly. Two sites get strictly better ordering: the metrics read guard in `run_metrics_push_loop` shrinks to the end of its initializer block (it never crossed an `.await` either way), and an EC test now closes the volume's descriptors before the `TempDir` removes the directory. No `rust-version` is declared. Edition 2024 needs rustc 1.85, but that is not the binding constraint — the dependency tree already requires 1.91.1 through the `aws-sdk-s3`/`aws-smithy-*` family, so `cargo +1.85 check` fails on the deps regardless. CI builds on `dtolnay/rust-toolchain@stable`. `vendor/reed-solomon-erasure` is a separate package and keeps edition 2021. Cargo.lock is unchanged despite edition 2024 implying resolver 3. Verified: `cargo test` 551 passed / 0 failed, `cargo test --no-default-features` 550 passed / 0 failed (the two feature sets produce an identical migration site list), `cargo build --release` clean. No automated test covers shutdown ordering, so the channel and runtime sites in `main.rs`, `write_queue.rs` and `grpc_server.rs` were read individually. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018nty5Rj7ssMQdFxHHjZgDC * rust volume: address edition-2024 review feedback Three fixes from review of the edition bump. Serialize the two environment-reading tests. The SAFETY comments on the `env::set_var`/`remove_var` helpers claim every test touching the environment holds `process_state_lock()`, but `test_resolve_config_defaults_dir_to_platform_temp_dir` and `test_resolve_config_index_accepts_redb_and_leveldb_aliases` called `resolve_config` — which reads HOME/USERPROFILE, SEAWEED_WRITE_QUEUE and the WEED_* set — without taking it. `set_var` is unsafe precisely because a concurrent *reader* is UB, not only a concurrent writer, so the comment was overclaiming. An audit of the module found exactly these two; every other environment-touching test already held the lock. The race predates edition 2024, which only made the requirement explicit. Declare `rust-version = "1.91.1"`. The edition needs 1.85, but that was never the binding constraint: `cargo +1.90 check --all-targets` fails on the `aws-sdk-s3`/`aws-smithy-*` family, and 1.91.1 checks clean. Declaring the verified floor turns a wall of per-dependency errors into one clear message. Cargo.lock is unchanged despite this making the resolver MSRV-aware. Update the README, which advertised "Rust 1.75+ (2021 edition)". 1.75 was already stale before this branch — the tree has needed 1.91 for a while. Verified: `cargo test` 551 passed / 0 failed, `cargo test --no-default-features` 550 passed / 0 failed, `cargo build --release` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018nty5Rj7ssMQdFxHHjZgDC * rust volume: state the exact MSRV patch release in the README The README said "Rust 1.91+", which reads as 1.91.0 and is wrong by one patch release: `cargo +1.91.0 check --all-targets` fails on the aws-sdk-s3 family, `cargo +1.91.1` passes. Say 1.91.1+, matching `rust-version` in Cargo.toml, and call out that the patch component is load-bearing so nobody installs 1.91.0 and hits the same wall. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018nty5Rj7ssMQdFxHHjZgDC * rust worker: move the workspace to edition 2024 Moves the seaweed-worker workspace (core, lance, sort) from edition 2021 to 2024, the same migration seaweed-volume just got in this branch. Edition 2024 turns exactly one thing in this workspace into a hard error. The baseline came from RUSTFLAGS='-W rust-2024-compatibility' cargo check --all-targets, run before the flip; unlike seaweed-volume's 34 silent + 8 hard sites, the worker reports only the one hard site and no tail_expr_drop_order or if_let_rescope sites at all. The worker is a much smaller crate and none of its expressions hold a guard or temporary whose drop order the edition changes, so there is nothing to audit on the silent side. Fixed (1 site): std::env::set_var is unsafe as of 2024 because it races with concurrent readers. The single call is in crates/core/build.rs, which sets PROTOC from protoc_bin_vendored the way seaweed-volume's build script does. A build script's main runs single-threaded before anything else in the process, so no other thread can be reading the environment concurrently; the SAFETY comment says so. There are no config.rs-style test helpers here -- the worker's tests do not mutate the environment -- so unlike the volume crate there are no process_state_lock() callers to audit. No redundant ref bindings to clean up: a grep for ref across the three crates finds none. MSRV: rust-version = "1.94.1", verified rather than inferred. Edition 2024 only needs 1.85, but the dependency tree needs more: lance's aws feature pulls in a newer cut of the same aws-sdk-*/aws-smithy-* family that sets seaweed-volume's 1.91.1 floor, and that newer cut requires 1.94.1. cargo +1.94.0 check --all-targets fails on that family; cargo +1.94.1 check --all-targets is clean. The worker's floor is therefore higher than the volume's, and moves with lance and the AWS SDK rather than with the edition. CI builds on dtolnay/rust-toolchain@stable, so nothing changes there. The edition is set once in [workspace.package] and inherited by each member via edition.workspace = true; rust-version is added the same way. The workspace keeps its explicit resolver = "2" -- edition 2024 would default to resolver 3, but the pin is deliberate and Cargo.lock is unchanged by this commit either way. The README gains a "Requires Rust 1.94.1+ (2024 edition)" line in its Building section, matching the one seaweed-volume's README now carries, and calling out that the patch release is load-bearing (1.94.0 does not build) so nobody installs 1.94.0 and hits the same wall. Verification: * cargo check --all-targets -- clean, zero warnings (default toolchain 1.97) * cargo +1.94.1 check --all-targets -- clean * cargo +1.94.0 check --all-targets -- fails on the AWS SDK, as claimed * cargo test --all-targets -- 40 passed, 0 failed (core 13, sort 11, lance lib 3, lance bin 2, compaction 6, lifecycle 1, sort integration 4) * Cargo.lock unchanged Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris LuandGitHub
|
2ffa696809 |
fix(volume): handle faulty storage media (Go + Rust) (#11233)
* fix(volume): track EC shard read errors and unmount on faulty media Extract the volume EIO tracker into a reusable IoErrorTracker and add the same tracking to EcVolume. Sustained EIO on .ecx lookups or .ecd shard reads now unmounts the EC volume in the heartbeat (without deleting files) so the master re-replicates from healthy peers, mirroring the existing volume replica quarantine. Closes #11227 (EC shard unmount). * rust(volume): mirror EC shard read error tracking and unmount Add EIO tracking to the Rust EcVolume mirroring Go: a streak counter with IO_ERROR_TOLERANCE, a sticky quarantine flag, and unmount (not file deletion) in the heartbeat so the master re-replicates from healthy peers. * feat(metrics): expose storage IO error counter and quarantine gauge Add a storage_io_error_total counter incremented on every EIO recorded by the volume or EC shard tracker, and an io_quarantine gauge labelled by kind (volume/ec_shard) reflecting the count of replicas suppressed in the heartbeat. Mirrored in Go and Rust. * feat(healthz): report 503 when local replicas are IO-quarantined Add Store.HasIoQuarantine (Go) / Store::has_io_quarantine (Rust) and have /healthz return 503 when any local volume or EC shard is quarantined due to sustained storage-media EIO, so a load balancer can drain a server whose underlying media is faulty. Mirrored in Go and Rust. * fix(volume): keep quarantined EC volumes in memory and reset EIO on success Address review feedback: instead of unloading quarantined EC volumes (which discards the quarantine state healthz needs), keep them in memory and just skip them from heartbeat reporting, mirroring the regular volume quarantine. Also clear the EIO streak on successful .ecx reads in Rust so a transient error does not accumulate, and add an ec_shard label to the io_quarantine gauge in both Go and Rust. * fix(volume): exclude quarantined EC shards from heartbeat and add Rust volume tolerance Address review feedback: - Filter quarantined EC volumes from CollectErasureCodingHeartbeat (Go) and collect_ec_shard_delta_messages / collect_live_ec_shards (Rust) so the master stops advertising faulty shards and re-replicates from healthy peers. - Add consecutive EIO count and sticky quarantine to the Rust regular Volume, mirroring Go IoErrorTracker: a single EIO no longer deletes the replica; the heartbeat quarantines after the tolerance threshold and keeps the volume in memory. - Use the quarantine flag (not last_io_error) in has_io_quarantine so /healthz reflects sustained, not transient, failures. * fix(volume): make Rust quarantined volumes read-only and wire recovery Address Devin review: - Set no_write_or_delete on Rust volumes when quarantined in the heartbeat, so cached or direct clients cannot mutate a faulty replica after the master removes it (mirrors Go). - Wire reset_io_error_state into Volume::set_writable so an operator making a volume writable again clears the sticky quarantine and the volume re-enters heartbeat rotation. * fix(volume): clear EC quarantine on shard re-mount for operator recovery Address Greptile review: re-mounting EC shards (Go loadEcShardWithIdxDir / Rust mount_ec_shards_with_idx_dir) now calls ResetIoErrorState on the existing EcVolume, giving operators a documented recovery path that clears the sticky quarantine and returns the EC volume to heartbeat rotation. Mirrored in Go and Rust. * fix(volume): do not clear EC quarantine on routine shard mounts Address review feedback: clearing the EC IO quarantine on every mount (including duplicate, retry, sibling-shard, and reconciliation mounts) is too aggressive and can re-advertise known-bad shards before the storage media has been validated. Remove the automatic reset from the mount path; quarantine clears naturally on restart or full unmount when a fresh EcVolume is created with clean state. * test(volume): update Rust IO error test for quarantine semantics The heartbeat now quarantines a volume with sustained EIO (keeps it mounted, makes it read-only, omits it from heartbeat) instead of deleting it. Update test_collect_heartbeat_deletes_io_error_volume to assert the volume stays in the store with no_write_or_delete set, and update set_last_io_error_for_test to set the consecutive error count at the tolerance threshold so the test reflects a sustained error. * fix(volume): reset EIO streak after full write and match Windows media errors Move the success-side EIO reset from append_needle (after write_all only) to the end of do_write_request, after flush_dat/flush_idx complete, so a successful write_all followed by a failed fsync no longer resets the counter before the EIO is recorded. Repeated fsync EIOs now accumulate toward the quarantine threshold as intended. Recognize Windows storage-media failure codes ERROR_CRC (23) and ERROR_IO_DEVICE (1117) in addition to Unix EIO (errno 5), so quarantined heartbeat behavior is preserved on Windows. Mirrors the change in both Go and Rust volume servers. * fix(volume): preserve checkpoint EIO and clear streak on successful delete maybe_checkpoint_index now returns whether the checkpoint succeeded; the success-side EIO reset in do_write_request and do_delete_request only fires when it did, so a checkpoint media failure is no longer erased by the unconditional reset that followed it. do_delete_request also gains the success reset that was lost when append_needle stopped clearing the streak, so a successful delete still clears an earlier failure streak. is_storage_io_error now uses libc::EIO on Unix instead of a hard-coded 5, and the ECX binary-search read path gains a Windows fallback (seek + read_exact) so the buffer is no longer zeroed on non-Unix targets. |
||
|
|
9b12d13934 |
volume server: release the store lock before scrubbing EC volumes (#11235)
* volume server: release the store lock before scrubbing EC volumes
`ec.scrub` makes a Rust volume server stop serving for the duration of the
scrub, and then kills its own gRPC connection:
error: rpc error: code = Unavailable desc = keepalive ping failed to
receive ACK within timeout
Measured on a 4.46 cluster (17 Rust volume servers on one host, ~520 volumes
and 53 EC volumes, --index=redb, EC 10+4). It reproduces against a SINGLE
node in 30-70s, in checksum, index and local modes, at -maxParallelization 1.
## Cause
The CHECKSUM arm of scrub_ec_volume reads every byte of every local shard
while holding the caller's store.read() guard:
let store = self.state.store.read().unwrap();
let ecv = store.find_ec_volume(vid)...?;
let (blocks, broken, errs) = ecv.checksum_scrub(); // GBs of I/O, lock held
VolumeServerState::store is a std::sync::RwLock, which is write-preferring.
The periodic heartbeat's collect_heartbeat_with_snapshot takes store.write()
and blocks; once that writer is pending, every later store.read() queues
behind it. Every HTTP handler takes store.read(), so the node serves nothing,
stops heart-beating, and cannot answer the scrub RPC's own keepalive - the
scrub kills the connection it is running on.
The INDEX and LOCAL arms have the same shape, and the node-wide scrub_volume
loop is worse: it held ONE guard across every volume on the node.
## Evidence
offcputime, off-CPU stacks >1s in a 30s window during a scrub:
futex_wait
seaweed_volume::server::heartbeat::collect_heartbeat_with_snapshot
- tokio-rt-worker
27967020 <- 27.97s blocked, of a 30s window
A single HTTP /status request issued 12s into a scrub, with 180s of patience,
was accepted and queued for 120 seconds, then served once the scrub released.
Thread states throughout: 1 D + 48 S. One thread working, 48 idle - not
executor starvation and no thread pileup, which is what a single lock holder
looks like.
Memory was tested and ruled out as the cause: the same scrub was run at
MemoryMax 3G, 8G and unlimited. With no limit there is no reclaim at all,
page cache grows freely to 22 GB, and the node still goes unresponsive at
t+30s. anon stays flat at 48-86 MB in every run.
## Fix
checksum_scrub, scrub_index and scrub_local gain plan types -
EcChecksumScrubPlan, EcIndexScrubPlan and EcLocalScrubPlan - snapshotted from
the volume under a brief guard. The handler builds a plan, drops the guard,
and runs the scan in spawn_blocking, off the async workers, since it is
synchronous CPU + file I/O either way.
A plan captures DESCRIPTORS, not paths. Resolving a path again after the
guard is dropped would let a writer that legitimately unlinks the files - the
heartbeat's delete_expired_ec_volumes, which reaches EcVolume::destroy(), or
volume_ec_shards_delete - surface an intentional removal as "scrub read
error: No such file or directory" and put the volume in broken_volume_ids. A
descriptor outlives the name.
For the shards it duplicates the handle the mounted EcVolumeShard already
holds (try_clone_file), which is what Go does: ChecksumScrub reads through
shard.ReadAt (weed/storage/erasure_coding/ec_volume_scrub.go:71), never
through a path. That also inherits open_volume_file's O_NOATIME and drops a
dead branch - the old code built {base}.ec{id}.v{gen} for a non-zero
generation, a name nothing in this tree writes. dup shares the kernel offset,
so shard reads stay positional; the .ecx gets a fresh open instead, since
check_index_file seeks.
FULL/READS is unchanged here: it already released the guard across the index
walk, and still re-takes it per needle in store_ec::scrub_snapshot_under_lock
for that needle's local shard intervals - short holds, many of them.
scrub_volume now takes the read guard PER VOLUME instead of across the whole
loop, so the heartbeat can land between volumes. Its per-volume work still
runs under the guard; Volume needs an equivalent plan to fix that properly,
left as a follow-up and noted in the code.
## A failed scrub task must not take the whole RPC down
Moving the scans into spawn_blocking changed where a panic lands. It no
longer unwinds inside the handler's own future; it comes back as a JoinError
at the .await, and all four join points sat behind a `?`. So one bad volume
out of six hundred returned Err from the entire handler: the
broken_volume_ids, broken_shard_infos and details already gathered for the
other 599 were dropped, and emit_scrub_metrics - the only writer of
SCRUB_LAST_TIME_SECONDS, SCRUB_VOLUME_FAILURES and SCRUB_SHARD_FAILURES - was
never reached, so the staleness alert kept firing while real corruption went
unreported.
And there is a reachable panic behind it. EcLocalScrubPlan::run() sized its
reassembly buffer with
Vec::with_capacity(get_actual_size(size, version) as usize)
which for any negative size that is not the -1 tombstone skipped above is a
capacity-overflow abort. Mode 3 (LOCAL) is the default of `weed shell
ec.scrub`, and a scrub is what you point at an index you already suspect, so
an arbitrary i32 in a .ecx size field is in-scope input. The buffer is
Rust-only - Go appends to a nil slice and has no capacity hint here. Guard on
`want <= 0` and fall through with an empty buffer: locate_data returns no
intervals for a non-positive size, read stays 0, and the existing
`read != want` error reports the row exactly as Go does.
Each join point now records the failure against its own volume and continues.
A panic is evidence about the volume and counts as broken; a non-panic
JoinError is not - spawn_blocking only reports one when the runtime is going
down, the volume was never scanned, and counting it would put a false
corruption into SCRUB_VOLUME_FAILURES. total_volumes moves before the join in
modes 1, 3 and 4 (2|5 already counted there) so a failed join cannot silently
shrink it. Mode 2|5's verify_ec_shards join is the one that must not
`continue`: the needle walk above has already produced findings for that
volume.
The tombstone guard stays is_tombstone() on purpose. ScrubLocal in
ec_volume_scrub.go:228 skips only IsTombstone(), while the distributed walk
in store_ec.go:516 skips all IsDeleted() - the asymmetry is Go's, and both
Rust walks mirror their own counterpart.
## Both servers: a node-wide scrub skips a volume that vanished mid-run
Releasing the lock makes the volume set legitimately mutable during a scrub,
so a node-wide run can reach a volume that has since been unmounted. That is
not a scrub failure. A node-wide run now logs and skips it; an explicitly
requested volume id still returns NotFound. The Go server is changed the same
way, so both implementations answer the same shell command identically.
mark_broken_volumes_readonly tolerates the same teardown one step later,
instead of throwing away the whole scrub report.
## Test
test_scrub_plans_are_self_contained_and_match_direct_call drops the EcVolume
and runs both plans on another thread, asserting the results match the direct
calls. A plan that borrowed from EcVolume could do neither, so the test stops
compiling if the snapshot regresses to a borrow.
test_scrub_plans_survive_files_removed_after_snapshot unlinks every shard and
the .ecx after the plans are built, then asserts the results still equal the
direct call. Against a path-resolving version it fails with all 14 shards
reported as "No such file or directory".
test_local_scrub_plan_reports_negative_size_ecx_row rewrites a .ecx row's
size to -1000 and runs the local plan on another thread, so the join is the
assertion - that thread is the spawn_blocking whose panic used to fail the
RPC. Without the capacity guard it fails with "capacity overflow"; with it,
the row is reported.
The Go tests cover both halves of the vanished-volume rule for volumes and EC
volumes.
517 lib tests pass, plus 34 across the other targets (`cargo test`).
`go test ./weed/server -run Scrub` passes.
## Known remaining, not fixed here
`ec.scrub -volumeId=N` is still fanned out to every node, and a node that
holds no shard of N returns NotFound, so the shell command errors even when
the nodes that do hold shards scrub cleanly. That is a shell-side fan-out
question rather than a volume-server one, and both servers keep the existing
behaviour for an explicitly requested id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DvHoW85w6SNKNBPvrqLMmK
* scrub: discard checksum block count from total_files; capture .ecx fd for FULL walk
Two review fixes:
1. CHECKSUM arm: plan.run() returns blocks scanned, not a file count.
Go discards it (_, shardInfos, serrs = v.ChecksumScrub()) so TotalFiles
stays a needle/file count. The Rust arm was adding it to total_files,
inflating the count. Discard it to match Go.
2. FULL/READS (scrub_ec_volume_distributed): the needle walk reopened the
.ecx by PATH after the store guard was released, so a concurrent teardown
that unlinks or replaces the .ecx (heartbeat delete_expired_ec_volumes,
volume_ec_shards_delete) could surface an intentional removal as a scrub
error or mix index generations within one scrub. Capture a second .ecx
descriptor under the guard (the index plan handle is consumed by its own
structural walk, and both seek) and read through it instead -- the same
descriptor-outlives-name invariant the checksum plan shard handles use.
* scrub: bind FULL/READS walk to one encode generation
Address Devin review: after capturing the .ecx descriptor under the guard,
scrub_snapshot_under_lock still re-resolves the volume by id per needle, so
a teardown-and-remount of the same vid between two rows would apply the
captured .ecx offsets to a replacement volume's shards -- falsely reporting
corruption.
Capture the volume's encode_ts_ns (encode-run identity) in Phase A and pass
it to scrub_snapshot_under_lock. If the mounted volume's encode_ts_ns no
longer matches, abort the walk like a mid-scan unmount instead of mixing
generations within one scrub.
* scrub: run FULL/READS index scan in the blocking pool
Address CodeRabbit review (5147767192): index_plan.run() reads the whole
.ecx synchronously, so running it on the async executor worker could block
unrelated RPC work handled on the same executor. Move it into spawn_blocking,
matching the treatment the CHECKSUM/LOCAL arms already give their plans. A
join failure (panic/cancellation) is reported as a seed error so the
per-volume findings below are not silently dropped.
* scrub: move ecx walk to blocking pool, classify join errors, guard encode_ts_ns==0
Three CodeRabbit review fixes (5148034447):
1. Move the FULL/READS needle walk (walk_index_file over the captured ecx
descriptor) into spawn_blocking. It reads the full .ecx synchronously and
was still running on the async executor worker, the same blocker the
index_plan.run() fix in the previous commit addressed.
2. Preserve JoinError classification in both spawn_blocking join points in
scrub_ec_volume_distributed. A panic is evidence about the volume and
counts as broken; a cancellation only happens at runtime shutdown, the
volume was never scanned, and returning it as an error would put a false
corruption into broken_volume_ids (the FULL/READS arm marks the volume
broken on any non-empty errs). Panics return an error; cancellations
return clean.
3. Do not treat encode_ts_ns == 0 as a verified generation match. The .vif
assigns 0 when it carries no encode-run identity (legacy/pre-feature
volumes), so 0 == 0 would accept a teardown-and-remount and apply the old
.ecx offsets to the replacement volume's shards. Only enforce the
generation check when the captured identity is non-zero; when it is zero,
fall back to the pre-check behavior (no generation binding) rather than
aborting a scrub that was already running without the guard.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
|
005012edcf |
rust volume: quick-repair redb on durable checkpoints (#11203)
* rust volume: insert redb rebuilds in needle-id order Unlink the .rdb before create (create does not truncate). Collapse last-write-wins, then insert live keys sorted so 4.2.0 packs leaves. * rust volume: rebuild redb from a BTreeMap and clear leftover keys Peak rebuild memory is one ordered map instead of HashMap + Vec + stable-sort scratch. Unlink stays best-effort: if it fails, retain clears the leftover table before sorted insert. Compute idx metrics before the write so a read error does not unlink a committed .rdb. * rust volume: drop the extra redb read transaction on put/delete put uses insert()'s previous value. delete gets then inserts the tombstone in the same write transaction. Truncate the .idx row on any failed redb write after the append. * rust volume: unpack redb blobs through packed_to_needle_value save_to_idx, ascending_visit, and collect_entries used the same length-check copy as get. Route them through the helper so a wrong-length value is absent everywhere, not a panic. * rust volume: quick-repair redb on durable checkpoints set_quick_repair(true) on the durable checkpoint transaction so an OOM-killed volume server opens without a full-file repair scan. * rust volume: reopen redb from .idx on non-poisoned commit error redb 4.2.0 can make a Durability::None commit visible before returning Err(CommitError::Storage(..)). In that state the database refuses further write transactions, so truncating the .idx row (the old behavior) would leave a redb-only put or tombstone that the stored idx_size makes the reload skip. Distinguish CommitError::TransactionPoisoned (txn rolled back, db still usable -- truncate the orphan .idx row as before) from other commit errors (change may be visible, db refuses writes -- keep the .idx row, close the database, and reopen from .idx to repair redb's internal state). db becomes Option<Database> so reopen_from_idx can drop the old file lock before load_from_idx opens the same path. rdb_path, version, and cache_bytes are stored so the reopen uses the same configuration. * rust volume: truncate .idx row when redb is closed in put put appends the .idx entry before acquiring the write transaction. When db_or_err() fails (db is None after a failed reopen), the ? returned without calling truncate_idx_to_offset, so a write reported failed remained in the authoritative .idx and was replayed on restart. Handle db_or_err() explicitly and truncate the orphan .idx row before returning the error, matching the existing handling for begin_write, open_table, and insert failures. --------- Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Eliah RusinandGitHub
|
ed5b342f0c |
rust volume: optional redb insert_before bulk load (#11205)
* rust volume: quick-repair redb on durable checkpoints set_quick_repair(true) on the durable checkpoint transaction so an OOM-killed volume server opens without a full-file repair scan. * rust volume: optional redb insert_before bulk load Behind redb-experimental-cursor (default off). Production binary stays on sorted insert(). CI unit tests run both feature settings. * rust volume: exercise insert_before across leaf splits Replace the 5-key cfg clone with a 4000-key reverse-order rebuild so CursorMut::insert_before hits page splits. CI runs the feature only on storage::needle_map unit tests. |