18 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
3 people	5d5fcdf07b	fix(filer): bound aggregated metadata reads by peer watermarks (#10803) ... * fix(filer): watermark-bound aggregated metadata subscription against multi-source merge races The aggregated metadata subscription (SubscribeMetadata) merges per-filer sources that become readable at independent paces, but tracks its progress with a single scalar cursor. Once the cursor passes a timestamp T, anything a source materializes below T afterwards is silently skipped: a peer recovering from a stall re-inserts its backlog late (late ring merge), and a source's flush can land a log file, or a later chunk of the same file, after a subscriber's disk pass listed the files (late persisted-log landing). This is the residual documented in #10501. Bound the subscriber's two read paths by what every source has provably made visible, each with its own watermark: - Delivery low-watermark -> in-memory reads. The meta aggregator tracks, per subscribed peer (self included), the newest timestamp received on that peer's stream - real events, or idle heartbeats (peer streams now opt into ClientSupportsIdleHeartbeat). The aggregated ring is complete up to the minimum across peers; in-memory reads hold at it. - Flush low-watermark -> persisted-log reads. Each filer reports its local log-buffer flush watermark on its stream: a new flushed_ts_ns response field, carried on idle heartbeats and on periodic flush reports (gated on ClientSupportsIdleHeartbeat). Disk passes freeze the minimum across peers before listing the log files and hold at it; the day-boundary cursor jump and the metadata-chunks ref listing are bounded the same way, the latter at minute-file granularity. - Held reads keep the cursor at the last entry actually delivered and retry; the retry re-lists the log files, which is what picks up a late-landing file. Both watermarks are relaxed by the settled horizon (2 x LogFlushInterval) as a liveness escape, so a peer stalled beyond it delays subscribers by at most the horizon instead of forever - any loss that escape allows was unconditional before. With reads held at the flush watermark, a disk advance below it is proven complete on every peer's disk, so the unproven-crossing counter now only counts crossings the horizon escape allowed past a stalled peer. Live delivery on the aggregated stream may lag by up to the idle-heartbeat interval when some peers are quiet; SubscribeLocalMetadata consumers are unaffected. * fix(filer): resume evicted aggregated readers from an original-space disk anchor The aggregated ring rewrites out-of-order peer arrivals to its head, so a subscriber tailing it advances its cursor in bumped (arrival) timestamps, while persisted logs keep original timestamps. When a slow reader's unread window is evicted (e.g. a peer backlog flooding in after a stall) and the reader falls back to disk, resuming from the bumped cursor skips every original-space entry below it that memory never delivered - reproduced as a ~66% silent loss on a 3-filer cluster with one peer's stream frozen for ~70s while the subscriber lagged. Track a disk anchor: the newest original-space position the stream is proven complete through. Disk passes advance it directly; contiguous memory reads advance it to the peers' delivery low-watermark observed before the read (per-peer streams are ordered, so everything with an original timestamp at or below that watermark had already arrived and was delivered). A reader kicked off the ring resumes the disk pass from the anchor instead of the bumped cursor - redelivering what memory already sent is within the subscription's at-least-once contract, skipping what it never sent is not. * fix(filer): close review findings on the peer-watermark subscription bounds Four correctness holes found in review, one generated-file cleanup: - The flush-through claim could assert durability for events still on their way into the buffer: an event is timestamped before notification work that can block, and only then appended. Track stamped-but-unappended events on the Filer (the stamp shares a lock with the reader, and appends are bumped monotonically past the buffer head), and cap the reported flush watermark just below the oldest in-flight stamp. - Removing a peer deleted its watermark entries while its stream kept running: its next signal recreated the deleted entry, which then pinned the low-watermark forever once the stream died. Watermarks now advance only for tracked peers, and peer removal cancels the subscription context so the stream stops feeding the aggregated buffer promptly. - The pipelined sender folded flush reports (TsNs 0 reads as far behind) into batch Events tails, where the aggregator's nil-notification guard dropped them - a busy backlog replay could starve the flush watermark until the settled-horizon escape opened a loss window. Control messages are now unbatchable on the sender, and the receiver also reads watermark state off nested batch entries as belt and braces. - A give-up skip's cursor was not anchored, so the next eviction rewind undid the counted decision and re-entered the same park forever when the evicted window carried bumped timestamps. The anchor now follows give-up skips; an anchored cursor makes the rewind a no-op and keeps the gap machinery's re-arm onto the retained window reachable. - Regenerated-file churn from a different protoc-gen-go-vtproto version is dropped: the vtproto file is upstream's, plus only the flushed_ts_ns marshal/size/unmarshal cases in the same generator style. New tests pin the in-flight floor, the no-resurrection rule for removed peers, and that control messages are never nested in batches. * fix(filer): keep a removed peer's watermarks through a grace period Deleting a peer's watermark entries the moment the master removes it reopened the loss the watermarks exist to prevent: a filer frozen or partitioned long enough to miss master heartbeats is removed from the cluster, its unflushed events still exist, and with its entries gone the low-watermarks snap forward to the healthy peers - subscribers advance past the absent peer's window and its late-landing log files are silently skipped. Reproduced on a 3-filer cluster: freezing two filers for ~70s got them removed ~28s in, and a catching-up subscriber lost their entire overlapping window. Removal now only marks the peer; its watermarks keep participating in the low-watermarks for a grace period (2 x LogFlushInterval, matching the subscribe loops' settled horizon, which already bounds a stale watermark's influence meanwhile). A re-added peer clears the mark and continues its values monotonically - the flap case costs nothing. A peer that stays gone is dropped when the grace expires, so a decommission cannot pin the low-watermarks, and a dropped peer's straggling signals cannot resurrect its entry. * fix(filer): cap delivery heartbeats by the in-flight floor; harden stamps Second review pass on the watermark bounds: - Idle heartbeats on the local stream claimed delivery-completeness through "now" while an event could still sit stamped-but-unappended behind blocking notification work. A peer aggregator turns that claim into its delivery low-watermark, so it could advance (and anchor credits with it) past an event that had not been streamed yet. The heartbeat timestamp is now capped just below the oldest in-flight stamp, like the flush claim already was. - In-flight stamps are forced monotonic against the registry's own history, so a wall-clock step backwards cannot slip a new stamp under an already-sampled floor. The cross-goroutine ordering still shares the meta log's global forward-clock assumption; the comments now say so instead of overclaiming. - Duplicate removal notifications no longer refresh a removed peer's grace deadline: the first removal time wins, so a decommissioned peer cannot sit in the watermark sets forever on repeated updates. - A failed buffer append clears the event's in-flight stamp on purpose: the event is dropped from the change stream entirely (a pre-existing defect of the append path, loudly logged), and a watermark waiting for it would pin this filer's claims forever. The comments now state the decision instead of implying the failure cannot happen. * docs(filer): tighten the watermark comments Comment-only: compress the narrative comments added on this branch down to their load-bearing invariants, and fix one stale sentence (peer removal no longer deletes the watermark entries immediately). No code changes. * fix(filer): subscribe to the local filer before remote peers Self's events reach the aggregated buffer only through the aggregator's own subscription to it, but bootstrap only seeded the peers the master already listed - and self's master registration races that listing, so the watermark set could hold remote peers without self. Once the remotes signalled, the low-watermarks would claim completeness for a stream that was still missing a merge source, letting aggregated subscribers advance past the local filer's events before its subscription started. Seed self first, unconditionally: before that the watermark set is empty (a documented safe state - reads hold at the settled horizon), and after it the set can never be remotes-only. The later master update for self, or a duplicate in the listed peers, is a no-op via the already-followed check in OnPeerUpdate. * fix(filer): fence watermark claims against wall-clock regression Record issued heartbeat/flush claims in the in-flight registry and stamp later events above them, so a backward clock step cannot land an event under a watermark a peer has already advanced to. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer): re-check the buffer head after fencing heartbeat claims An event appended between the caught-up check and the delivery claim was covered by the claim but not yet sent on the stream. The claims fence later stamps, so re-checking the head after them proves every covered event was already sent before the heartbeat. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer): cross the aggregated ring's pre-subscription range only on proof The eviction gate and the gap proofs read "nothing evicted yet" as "memory holds everything after the cursor". That is false for the merge-fed aggregated ring, which is born empty while every peer's history sits on disk: before the ring's first real eviction, a subscriber whose cursor was still below the bounded chunk pass's listing stop was served the ring's earliest entry inclusively, silently skipping the withheld pre-restart files - and the idle-wait callback credited the delivery low-watermark to the disk anchor in the same disconnected state. Mark everything at or below the subscriptions' start as evicted when the aggregator is built, credit the anchor only once the run is connected to the ring, and give the aggregated gap pass a real proof to cross the marked boundary with: each disk pass's proven coverage (the peer flush low-watermark capped by the pass's listing bound). An empty pass whose proof reaches the eviction watermark crosses to it silently - no park, no loss counter - so the mark costs a bounded catch-up delay instead of the 15-minute give-up. * fix(filer): keep shipped chunk tails at or below the hold point A log file spans past its named minute (window start plus up to a flush interval), and chunk-mode clients apply a shipped file whole - so a file tail past the hold point can become a persisted client checkpoint beyond what every peer has proven, and a crash inside that window resumes past another peer's late-but-in-contract flush. Stop the ref listing a minute plus a flush interval below the hold; the withheld band is served by the memory pass (ring retention far exceeds it) or by later passes as the hold advances, so freshness is unchanged. A frozen peer flushing one window that spans its whole freeze can still overshoot; that residual is bounded by the freeze and needs a crash inside it. * docs(filer): trim the review-fix comments --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-08-19 18:38:46 -07:00
3 people	813a4b0711	fix(filer): stop skipping recent unflushed events on metadata subscription gaps (#10501) ... * fix(filer): don't skip unflushed events on metadata subscription gaps A subscriber that falls behind the in-memory log ring during a write burst could have its read position jumped past events that were evicted but not yet flushed to disk — silently lost for filer.backup/filer.sync/ mount subscribers. Route all three gap-skip sites through resolveDiskGapResume: only skip past windows older than a settled horizon (2*LogFlushInterval); recent gaps wait for the flush and re-read disk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer): harden metadata gap-skip guard Address review findings on the settled-horizon guard: - local subscriptions: gate the skip on the buffer's flush watermark observed before the disk read (resolveLocalGapResume) — a disk miss is then proof the gap is empty, with no wall-clock assumptions - aggregated subscriptions: cap skips strictly below the horizon boundary (persisted reads exclude ts <= cursor) and pace capped advances, so the sliding horizon cannot cause disk-probe spinning - replace unbounded sync.Cond waits with a bounded select on the buffer's subscriber channel + retry timer + ctx cancellation, eliminating the lost-wakeup stall Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer): close remaining gap-skip loss paths from re-review - log_buffer: a sentinel-offset (time-based) read below the earliest in-memory entry silently started at earliest, skipping a window that may hold evicted-but-unflushed events. Track the ring's eviction watermark (lastEvictedTsNs) and keep the inclusive fast path only when nothing at/after the position was ever evicted; otherwise return ResumeFromDiskError so the subscription gap guard decides. - capped horizon advances stay on the disk-probe path (never expose a mid-gap position to the memory read) and keep pacing - gap jumps land just below earliest: positions are exclusive, so the earliest entry itself is still delivered - subscriber notification keys include clientId/epoch so a replacement stream never inherits a channel the old stream's cleanup closes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(log_buffer): generalize the eviction-watermark read gate Third-review findings: - epoch/zero-time reads bypassed the eviction watermark: gate them the same way, so a SinceNs=0 subscriber cannot silently start at the earliest retained entry after an unflushed window was evicted - apply the watermark gate regardless of the cursor's batch offset (batch offsets carry no meaning for time-based reads); this also serves adjacent cursors (earliest == current+1) from memory instead of stalling them in the gap loop - LoopProcessLogData reader names include clientId/epoch, since they are registered as subscriber keys internally (same collision as the outer notification keys) - test: pin the gate (below/at watermark, epoch-after-eviction) and update the slow-consumer test to the sharper contract — complete in-memory history is served from memory; disk only once evicted Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(log_buffer): watermark equality is unsafe for inclusive sentinel cursors Sentinel (Offset <= 0) time-based cursors search from ts-1ns, i.e. they read inclusively of their own timestamp — and the evicted window may end exactly at that timestamp. Allow watermark equality only for exclusive (positive-offset) cursors; sentinel cursors must be strictly above it. Also shut down the test buffer and pin the equality cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer): wait on an empty aggregated buffer instead of falling through When a disk read finds nothing for a ResumeFromDiskError gap and the aggregated buffer has no readable entries (zero earliest time), resolveDiskGapResume declines to advance and control fell through to the in-memory read — which returns ResumeFromDiskError again immediately, spinning through the probe cycle without any wait. Fold the case into the existing recent-gap branch so it waits (notification, cancellation, or the retry interval) before re-probing, matching the local subscription path, which already waits unconditionally. * fix(filer): serve the exact eviction-boundary entry without another flush wait When the flush watermark has passed the earliest in-memory entry but that entry sits exactly one nanosecond above the cursor, the exclusive resume target collapses onto the cursor and resolveLocalGapResume declines to advance — while a sentinel cursor at the eviction watermark keeps deferring the in-memory read to disk. Progress then depends on the next flush cycle. Re-arm the cursor with a positive (exclusive) offset instead: ReadFromBuffer explicitly allows positive-offset cursors at the watermark, so the boundary entry is served from memory immediately. Also promote the aggregated path's horizon-capped gap skip to a warning: that skip may pass events a stalled flush lands later (the aggregated ring has no flush watermark to gate on), so operators should see when a flush stall outlasts the settled horizon. * fix(filer): resume a gap skip with an exclusive cursor The resume position landed one nanosecond below the earliest in-memory entry but kept the inclusive sentinel offset. When that timestamp is also the eviction watermark, the read gate answers ResumeFromDiskError for an inclusive cursor, the disk read finds nothing, and the resume target collapses onto the cursor, so neither helper can advance: the subscriber parks on timed waits forever. A skip is only taken once the gap is proven empty, so nothing remains to deliver at the resume timestamp. Resume exclusively instead, and let an inclusive cursor already sitting on the target count as progress. * fix(filer): gate aggregated gap skips on eviction, not wall clock Wall-clock age never proved persistence. The aggregated ring has no flush watermark because peers persist their own local logs, so a horizon of 2 * LogFlushInterval was standing in for one. While the disk stayed empty the horizon kept sliding forward and walked the cursor past evicted-but-unflushed events one window at a time - the volume-outage stall this change set exists to survive. The ring does carry a real proof: its eviction watermark. If nothing at or after the cursor was ever dropped, memory still holds every entry after it and the gap is provably empty. Below the watermark entries were dropped and only the producing peer's flush can supply them, so wait for that flush instead of advancing. The skip that remains is the one the ring can prove, which keeps the infinite-loop guard for a genuinely empty gap. * perf(filer): stop re-probing disk on every metadata append A subscriber parked on a gap woke on the log buffer's subscriber channel, which fires on every append. On the aggregated path each wake re-ran a full ReadPersistedLogBuffer - a ListDirectoryEntries plus a readahead goroutine - so one parked subscriber turned every cluster-wide metadata event into a store query, exactly while the cluster is already struggling with the flush stall that parked it. An append also cannot settle the gap: what this waits on is a peer persisting its own log, which nothing local signals. Wait on the timer alone there. The local buffer does signal its flush on that channel, so keep it, but drain the stale token first: otherwise the appends riding the same channel spin the wait at write rate. The retry interval covers the notification the drain discards. * fix(filer): surface a metadata subscriber parked on a gap Refusing to skip an unresolved gap trades silent loss for a silent stall, and a stall is no easier to diagnose: filer.sync and mount followers just stop advancing, with no error on either end. The only trace was a V(3) line nobody runs with. Warn on entry and once a minute after, and carry a subscribe_gap_stalled gauge for the duration, so a flush that never lands shows up as a stalled subscriber rather than a consumer that mysteriously went quiet. * refactor(filer): drop the metadata listener cond with no waiters left Both listenersCond.Wait() sites are gone, so listenersWaits never leaves zero, the guard in the filer's notify callback never fires, and the two Broadcast calls around client registration wake nobody. Remove the cond, its counter and its lock, and pass a nil notify func: the log buffer already skips a nil one, and the subscriber channels carry these wakeups now. * test(log_buffer): drive the eviction watermark through a real seal Both eviction tests wrote lastEvictedTsNs directly, leaving the one line that sets it uncovered: copyToFlushInternal has to read slot 0 before SealBuffer shifts it out, and moving that read one statement later still passed every test. Append past the ring instead, and check the watermark appears only on the seal that drops a window, matches that window's stop, and then advances. The buffer under test never flushes, matching the aggregated meta ring where the watermark is the only emptiness proof. * refactor(filer): build the subscriber reader name once per stream It was rebuilt on every loop iteration from values that cannot change for the life of the stream. Hoist it next to the notification key it mirrors. * docs(filer): tighten the gap-handling comments Several ran to eight lines restating the same reasoning at each site. Keep the non-obvious why, drop the retelling. * fix(filer): mark a confirmed disk position as an exclusive cursor A disk read returns the timestamp of the last entry it handed to the subscriber, but the cursor built from it stayed inclusive. Land that cursor exactly on the eviction watermark and the read gate sends it back to disk for an entry the disk just delivered; the re-read finds nothing, neither emptiness proof holds for an inclusive cursor there, and the subscriber parks - until some later flush, or forever while one stays stalled. Everything after the watermark was sitting in memory the whole time. Carry the offset instead, so it also covers the ring evicting onto the cursor after the read rather than before it. The constant is no longer gap-specific, so it is now named for what it asserts. * perf(filer): re-park a gap wait woken by an ordinary append Draining one stale token did not bound anything: the subscriber channel carries an append per metadata write, so under continuous writes the next one satisfies the wait immediately. During the write burst these stalls come from, each parked subscriber ran the recovery loop at write rate rather than the intended two-second cadence. Only a flush can settle a gap, so check the flush watermark on wake-up and go back to waiting if an append is all that arrived. The retry timer is created once, so re-parking does not extend the interval. * fix(filer): keep a disk-derived cursor inclusive Marking every disk position exclusive assumed the entry at that timestamp was the only one there. On the aggregated stream it is not: disk can deliver one filer's persisted event at T while another filer's event at the same T is still unflushed in the aggregate ring. The exclusive cursor skipped it, and since the persisted reader also excludes ts <= its start, nothing would ever bring it back. Take the weaker guarantee instead. The reason the exclusive cursor was introduced - an inclusive one landing on the eviction watermark parks forever - is better answered in the resolvers: at the watermark memory holds nothing (retained windows start strictly after it) and the persisted reader cannot return that entry at any later time either, so refusing the gap buys nothing and never ends. Skip it whatever the cursor's inclusivity. That proof does not depend on a flush, so the local resolver takes it as a second, independent disjunct alongside its flush watermark. * perf(filer): wake a parked gap wait on flushes only Re-parking on an append bounded the work but not the wake-ups: the subscriber channel carries one per metadata write, so a parked subscriber still took a scheduling round-trip per write. Worse, draining it kept the channel empty, so every writer's non-blocking notify succeeded instead of falling through - the burst paid for the wake-ups too. Give the log buffer a flush-only subscriber list, notified from loopFlush, and park on that. The append channel now fills once and stays full, which is exactly the state the non-blocking send is designed for. * fix(filer): stop the gap-stall gauge from leaking a series per connection clientName is req.ClientName + "@" + peer address, so it carries the client's ephemeral source port and changes on every reconnect. Labelling the gauge with it minted a new series per connection, and clearing it only ever Set(0), so nothing was ever released - a client in a reconnect loop grows the filer's metric map and /metrics payload without bound. Key it on the stable client-supplied name, as the neighbouring subscribe gauge already does, and delete the series on teardown. Two logging fixes ride along, both in the same reporter: the resume warning was unpaced while the park warning throttles to one a minute, so a burst that parks and resumes every couple of seconds warned on every cycle; and clear() doubled as the teardown path, announcing "resumed after 14m0s parked" for a client that actually gave up and disconnected still behind. * fix(filer): give a parked gap wait the exits the read loop has A park never re-enters the read loop, so every exit that loop relies on stopped working while a subscriber was parked. It kept scanning the filer store every 2s for a client that a higher-epoch reconnect had already superseded, until the TCP connection finally died - hours, on a half-open one. It never reached the only code that honors UntilNs, so bounded callers like `weed shell fs.verify` and `filer.meta.tail -until=` hung instead of exiting. And a notification channel closed out from under it turned the bounded wait into a spin, since a receive on a closed channel returns instantly. Check all three where the subscriber actually waits. Bound the wait too: waiting is productive while the window is still queued for flush, but a peer that never returns, or whose filer store this filer cannot read at all, makes it permanent - and a subscriber that silently stops delivering is no better than one that silently skips. Fail the stream after that instead of hanging, loudly enough to say which gap and for how long. * fix(log_buffer): keep the eviction gate out of the shared read path The gate belonged to the filer's subscribe loops but was installed in ReadFromBuffer, which the message queue shares and which has no gap handling of its own. Three MQ paths broke on it: GetUnflushedMessages asks for everything in memory past the flush watermark and got ResumeFromDiskError instead, so SQL results silently dropped unflushed rows; a RESET_TO_EARLIEST consumer's epoch cursor was sent to disk, and the MQ disk reader resets an empty read back to epoch, so the whole partition replayed on every pass; and a disk cursor landing on the watermark carried offset -2, which the gate refused and the disk reader's ts <= start filter also skips, so neither side could ever serve it. The rewrite also dropped the old Offset <= 0 requirement, letting a stale positive-offset cursor jump to memory over on-disk history, and refused any negative-timestamp cursor even with nothing evicted. Restore the read path exactly as it was and keep only lastEvictedTsNs, which is the useful primitive. The filer loops now consult the watermark themselves before reading memory, which is where the gap handling that makes the refusal actionable already lives - and they check it every pass, not just when the disk came up empty, since a disk read can leave the cursor short of the watermark too. * fix(filer): read the log file whose window spans past its own name A log file is named for the start of the window it holds, but a window runs up to a flush interval longer, so "12-30" can hold entries through 12:31:58. File selection compared the cursor's minute against that name and skipped anything sorting earlier, so a subscriber resuming at 12:31:10 never saw the rest of that file: the read reported nothing on disk while the entries sat in it. That was survivable when a miss only meant "wait and retry", but the gap resolvers now read a miss as proof the range is empty and move the cursor past it, which turns those entries into silent loss. Start the file scan a flush interval early; entries are still filtered against the exact cursor, so this only opens one more file and never re-delivers. * fix(filer): resume a gap with a cursor the memory read will serve Reverting the eviction gate put ReadFromBuffer back to refusing every positive-offset cursor below the in-memory window, but the resolvers still handed back one - earliest-1 marked exclusive. So the resume bounced straight to ResumeFromDiskError, the disk had nothing, and the resolver saw its own cursor as no progress and parked: a subscriber stalled, and after the new bound failed outright, with the whole gap sitting in the ring the entire time. The exclusive marking only existed to dodge a park the gate itself caused, and the gate is gone. Resume at earliest with the sentinel offset, which is the position master used and which case 2.1 reads inclusively. That makes the resume unconditionally ahead of the cursor, so the progress check it needed goes away with it. * fix(filer): stop dropping a log file whose window outruns its name Listing the earlier file was not enough: the iterator then decided whether to read it by comparing the *following* file's name against the cursor, which treats that name as an upper bound on this file's contents. It is not one. A file is named for the start of the window it holds, minute-truncated, and the window runs up to a flush interval longer, so "12-30" can hold an event at 12:31:20 while "12-31" sits right after it - and a cursor at 12:31:10 skipped straight past the event. Bound the decision on the file itself: skip it only when its name plus the minute truncation plus a flush interval still lands at or before the cursor. That also covers the last file in the queue, which the old check never skipped because it had no successor to compare against. * fix(filer): stop a chunk-ref read from rewinding the subscriber CollectLogFileRefs reports the minute-level name of the last file it shipped, and the caller assigns that straight to the read position. Since the scan now reaches back a flush interval to catch a spanning file, a request at 12:31:10 that picks up the 12-30 ref moved the cursor to 12:30:00 - so the memory read that followed replayed events older than the client's own SinceNs and re-sent what the chunk reader had already been handed. The same rewind was reachable before, within a minute, whenever the last file's name sorted behind the request. Clamp the reported position to the one that was asked for. It still under-advances by design, since the server never reads the entries it ships refs for and cannot know where they end. * fix(filer): resume below earliest so a one-entry window is not skipped Moving the resume onto earliest itself was wrong for the smallest window there is. A sealed window holding a single entry has startTime == stopTime, and the sealed-buffer lookup only enters a window whose stopTime is strictly after the cursor, so a cursor sitting exactly on earliest walks past it and its sole event is never delivered. Low-volume metadata windows are routinely one entry, which is precisely when losing it is hardest to notice. Go back to one nanosecond below, which takes the startTime.After branch and returns the whole window, and keep the sentinel offset the memory read requires. The earlier test used an active multi-entry buffer, where both cursors happen to work; the new one seals single-entry windows and compares which entry comes back. * fix(filer): count a persisted read as progress only when it moves A chunk-ref read reports the minute-level name of the last file it shipped, now clamped so it never rewinds, so it comes back non-zero even when it names the position the subscriber already held. The loops read non-zero as progress: they cleared the stall timer, then found the cursor still short of the eviction watermark and parked again. Every retry re-shipped the same refs and reset the timer, so the bound that is supposed to end an unrecoverable stall was never reached - and a chunk-capable client buffers those refs waiting for an event that never comes, so it just accumulates duplicates. Require the reported position to be strictly ahead of the cursor. * fix(filer): count the evicted ranges the aggregated stream cannot prove The eviction watermark belongs to the merged ring, but the disk it gets checked against is the union of every peer's own log and each peer flushes on its own schedule. A read that lifts the cursor from below the watermark to above it may have done so entirely on a peer that is already ahead, while a lagging peer still holds unflushed events inside the range just crossed; when it flushes them they sit behind the cursor and are never delivered. One aggregate maximum is not proof that every peer persisted the range. Nothing available locally separates that from the ordinary case where every peer had in fact persisted it: the aggregator tracks peers by address while log files carry a random per-filer id, so "has this peer flushed through T" cannot be answered here at all. Deciding it needs the source filer's own flush watermark carried on the subscribe stream, which is a wire change this does not make. Count and log the crossing so the window is at least measurable instead of invisible. * fix(filer): send log file refs through the pipelined sender sendLogFileRefs wrote on the raw gRPC stream while pipelinedSender's goroutine concurrently calls Send for queued memory events - two senders on one stream, which gRPC forbids. The window used to open once per fall-behind; the gap retry loop now reopens it every pass. Routing refs through the sender also restores ordering: refs used to overtake up to 1024 queued events, and the client treats any non-ref message as the signal to process buffered refs, so overtaken refs were applied against the wrong position. The reason refs bypassed the sender was the batcher: their TsNs of 0 reads as far behind, and the client recognizes refs by the top-level field alone - a refs envelope would drop its Events tail, and refs inside Events would be applied as an empty event. Teach the batcher instead: refs messages always go solo, and one drained mid-batch is sent solo right after that batch. * fix(filer): count parked subscribers instead of flagging them by name The per-client gauge series could not work. Its label was rebuilt from the peer address at first, which leaks a series per reconnect; keyed on the client-supplied name instead, it collides - every mount registers as "mount" - so one stream's teardown deleted a parked sibling's live series, and the sibling never re-created it because its own park state said the gauge was already set. Either way the alert this gauge exists to drive goes dark. A count needs no identity: Inc on park, Dec on resume or teardown, scope as the only label. Client details stay in the logs. Also start warning only once a stall has outlived the warn interval. park() warned immediately on every first park, so catch-up churn that parks and resumes every couple of seconds logged a warning pair per cycle - exactly the flood the pacing was supposed to prevent, burying the long-stall warnings that matter. * fix(filer): one gap resolver, and re-arm an unservable adjacent cursor The two resolvers were the same function - the aggregated one is the local one with a flush watermark of zero, since its ring never flushes - duplicated down to the comment justifying the resume target. A fix applied to one and not the other is how the two streams drift; merge them. The merge carries the one behavioral fix both copies needed. Timestamp collision bumps make adjacent entries exactly 1ns apart, so an entry ending an evicted window leaves the cursor exactly one below earliest with a positive batch offset. The resume target then equals the cursor and both copies refused it as no progress - but that cursor cannot be served (ReadFromBuffer refuses positive offsets below the window) while the sentinel resume at the same timestamp is, and both deliver exactly the entries after it. Refusing parked a subscriber whose data was entirely in memory: until the next flush locally, and through a 15-minute stall failure on the aggregated path. Advance on equal target when the held cursor is exclusive; a sentinel there is already served, so it still refuses. * fix(filer): a bounded subscription parked exactly on UntilNs is finished The park's UntilNs check was strict while the bound is inclusive and cursors are exclusive: a disk read whose last entry sits exactly on UntilNs leaves the cursor there with everything up to the bound already delivered. If the next range was an unprovable gap, the completed subscription parked anyway and eventually failed - fs.verify hanging and then erroring on a healthy cluster. * fix(filer): re-derive the subscribe loops as one state machine The loops had grown three generations of gap handling - a post-disk-read branch, a post-memory-read branch, and an every-pass guard bolted on in front of the memory read - each consulting state the others mutated. The worst interaction wedged the aggregated stream permanently: diskExhausted compared the disk result against the cursor that result had just updated, and paired it with a ResumeFromDiskError latch that only a resolver advance cleared, so one fall-behind sent every later pass into the gap block and LoopProcessLogData never ran again. Mounts kept a healthy-looking stream and applied nothing. Both loops now run the same derived sequence. One disk pass; progress is the pre-update cursor against the result, freshly each pass. One gap decision before the memory read: a cursor the ring evicted past either keeps draining the disk (it just advanced), resolves forward (the gap is proven empty), or parks - and a cursor memory refused with nothing evicted after it re-arms onto the retained window. The stale-latch branch is gone: the error is only consulted on a pass whose own disk read came up empty. All four parks go through one parkOnGap helper, so the exits live in one place. The eviction watermark is read after the disk read and the same value feeds both the guard and the unproven-crossing report, which previously compared against a snapshot taken before a potentially minutes-long backlog read and missed evictions landing during it. The next-day jump now clears the stall reporter - it used to leave a stale park epoch that could kill the next brief park instantly at the 15-minute bound - and counts its own watermark crossing. Chunk-ref reads get two rules the old loops lacked. Refs are sent once per position: retries re-sent identical batches every two seconds into a client that only drains them on a non-ref message, growing an unbounded pending list. And when refs cannot advance the cursor - they report file start minutes, which sit below the content the client was actually given, pinning the cursor under the watermark forever during bursts - the pass falls through to entry reads, which move the cursor by real timestamps and double as the client's drain signal. * fix(filer): give up on an unprovable gap instead of failing the stream Failing after maxGapStall assumed the client could do something better, but every consumer just reconnects at the same SinceNs and hits the same wall, so an unprovable gap - a dead peer, or a peer whose filer store this filer cannot read at all - turned into a permanent 15-minute fail/reconnect loop delivering nothing. Master handled the same state by skipping instantly and silently. Take the middle: wait the full bound, then abandon the gap and resume at the eviction watermark, where everything retained starts strictly after, so the loss is exactly the range that could not be proven. The skip shares the unproven-crossing counter and logs at error level - loss is bounded, recorded, and the stream keeps working. A stall with nothing evicted past the cursor loses nothing by waiting, so it restarts the clock and keeps parking rather than skipping. * test(filer): pin the file-skip bound through the production predicate The spanning-file test asserted against its own copy of the arithmetic, so a regression in the iterator - restoring the next-file-name comparison or dropping the flush-interval term - would keep CI green while re-introducing the silent loss the fix closed. Extract the bound into logFileMayContainAfter, call it from the iterator, and point the test at it; breaking the production expression now fails the test. * test(log_buffer): pin the flush-subscriber contract The registry the filer's gap parks wait on had no test at all. Cover the observable contract: an append never wakes a flush subscriber, a flush does with the watermark already stored, unregistering closes the channel so an abandoned waiter unblocks, and double or unknown unregisters are harmless. The store-before-notify ordering in loopFlush is what makes the parks' wake-up re-check sound, and it is not black-box testable - reordering leaves a same-goroutine window of nanoseconds that hundreds of tight round-trips never catch. Mark it load-bearing at the site instead; a reorder now at least has to argue with the comment it deletes. * fix(filer): a -1 SinceNs is a position, not the refs-gate sentinel The once-per-position refs gate used -1 as "never sent", but a client may legally subscribe with SinceNs=-1, whose cursor timestamp is exactly -1: the very first pass then believed refs were already sent there and fell back to streaming the whole persisted history entry by entry - the bootstrap load chunks mode exists to avoid. Use MinInt64, which no cursor can carry. * refactor(filer): drop the aggregator's listener cond with no waiters left Same shape as the FilerServer cond already removed: nothing increments ListenersWaits and nothing ever calls Wait, so the three Broadcasts wake nobody and the notify callback's guard is always false. Aggregated subscribers wake through the buffer's subscriber channels now. * fix(filer): make the gap metrics say what they count The crossing counter's help text described only the aggregated peer case, but give-ups increment it for local stalls too - a wedged local flush - which sends an operator chasing peer replication when the problem is the local store. Label it by scope and say both. The stalled gauge counted every park, including waits with nothing evicted and nothing at risk, while its help text promised evicted-but-unpersisted events; describe it as what it is, a count of subscribers parked on a gap. * test(filer): make the flush and stall tests assert what they claim The flush-subscriber rounds were vacuous: the probe entry sat above the round timestamps, so every round entry was collision-bumped and the stored watermark exceeded the local value each assertion compared against - the same bump mistake this test suite already made once. Put the probe below the rounds and guard each round against bumping, so a vacuous setup fails instead of passing. The stall-outcome test wrote the reporter's park epoch directly, bypassing the gauge Inc that gaveUp() later Decs - leaving the shared process gauge at -1 for every test that runs after it. Park through the real path, age the park by hand, release what the test holds, and assert the gauge lands back where it started. * fix(filer): finish the stream checks before marking it parked parkOnGap stamped the reporter before waitOnGap ran its instant done exits, so a bounded subscription completing inside a gap state was marked parked for the microsecond before done fired - a phantom gauge blip and a false "disconnected still behind" warning on every healthy completion. Fold waitOnGap into parkOnGap so the done exits run first and the park mark only ever covers a stream that actually waits. While the park owns its timer, back the retry off as the stall ages - 2s probes growing toward one a minute - since every retry re-reads the persisted log, and probing the store each 2s for 15 minutes per parked subscriber during the very outage that parked them makes the bad time worse. The subtest still named for the old fail-the-stream stall behavior goes with the merge. * fix(log_buffer): gate filer cursors against eviction under the read lock The subscribe loops checked the eviction watermark and then read memory, but a seal can land between the two: the read then served a sentinel cursor from the earliest retained window, silently skipping the window just evicted - the loss class this PR exists to make loud, surviving as a race. The only place the check is atomic with the serve decision is inside ReadFromBuffer, under the lock seals take to evict. Rather than put the policy back into the shared read path - which broke four message-queue readers last time - add a new sentinel offset that opts into it: EvictionGatedOffset reads inclusively exactly like -2, except below the watermark it is refused to disk. The filer loops stamp it on every cursor they hand the memory read; a refusal lands in the same gap machinery the loop-side check feeds, so the race collapses into the handled path. MQ cursors never carry it and keep master behavior byte-for-byte. * fix(filer): gate the aggregated gap on received, not bumped, timestamps The aggregated ring rewrites an out-of-order arrival to its head plus a nanosecond, so after any bump-heavy interval - a peer history replay following a restart is enough - its eviction watermark lives above every timestamp that exists on any peer's disk. Comparing a disk cursor against it parked subscribers that had in fact drained every peer's log: a 15-minute delivery freeze ending in a give-up skip and a false loss alarm, on a healthy cluster where master resumed instantly. Track a second watermark in the received timestamp space - the highest pre-bump timestamp among evicted entries - and gate the aggregated loop on that. Disk cursors and received timestamps are the same space, so the comparison means what it says: at or past it, every evicted entry's original was at or below the cursor, and everything flushed of them was already delivered. The bumped watermark keeps guarding the in-ring read gate, whose cursors live in ring space. The local buffer is untouched: it flushes its own bumped timestamps, so there the two spaces are one. * fix(filer): ship each log chunk once and stop echoing ref'd files inline Chunk mode duplicated data through two doors. Consecutive ref collections overlap by design - the scan backs off a flush interval to catch a spanning file, and a filer appends chunks to its newest file - so the same file was shipped again on every pass that re-listed it: the client re-downloaded its chunks, and a duplicated file mid-batch rewinds timestamps inside the client's per-filer merge, which reads each stream as sorted - transiently resurrecting deleted entries during catch-up. Track per subscription how many chunks of each file were shipped and send only the unsent suffix; state prunes with the scan window, so it holds a few files per filer. The second door was the entry fallback: when refs cannot advance the minute-named cursor, the pass streamed the ref'd file's tail inline, and the client applies inline events unfiltered - the same tail it already applied from chunks. Entry passes for chunk clients now advance the cursor without delivering; everything they skip is covered by the refs already sent or the deltas the next collection ships. * refactor(filer): one gap decision shared by both subscribe loops The post-disk gap tree - guard, drain, resolve, two parks, the re-arm - existed twice, differing only in buffer, watermark space, flush getter, park channels, and reason strings. Four rounds of review fixes have shown the copies drift the moment one is edited alone. gapPass now carries the five differences and the tree lives once; the loops shrink to a three-way switch between reading memory, restarting the pass, and ending the stream. * docs(filer): trim the gap-machinery comments to the why Several blocks had grown to ten-plus lines restating what the tests already pin or retelling one rationale at multiple sites. Keep the non-obvious why - the load-bearing flush ordering, the two timestamp spaces, the refusal-at-equality argument - in a few lines each. * fix(log_buffer): credit an entry's received timestamp to its own window The received-ts capture ran before the rollover check, so an append that sealed the previous window stamped its timestamp onto that window and then lost it in the reset of the new one. The eviction watermark this feeds broke both ways: the sealed window's value was inflated by an entry it does not contain - parking aggregated subscribers on gaps that were drained - and the entry's real window was deflated, proving gaps empty that still held its event on some peer's unflushed path. Credit the timestamp only after the entry lands, when its window is known. * fix(filer): rebase a shipped chunk suffix to logical offset zero A grown file's delta kept the chunks' original file offsets, but the client's chunk reader starts at logical zero and a list opening higher reads as instant EOF - a successfully empty replay, and since chunk clients no longer receive disk entries inline, the appended events were silently dropped. Clone the suffix chunks with offsets rebased to zero; the cut is record-aligned because each append is one uploaded chunk of whole entries, so the suffix decodes as a file of its own. * fix(filer): finish every chunk refs batch with a transition the client acts on Both chunk consumers buffer refs until a non-ref message arrives, so a source with historical logs and a quiet ring - a mount reconnecting after a filer restart is the common case - shipped its backlog and then went silent: the client sat on the refs until the next metadata mutation anywhere in the cluster. The disk step now ends every batch with the empty-notification marker the client already treats as a resume-cursor advance. The same step closes the inline replay: the cursor used to stay at the last file's minute name, so the memory read re-delivered the retained tail of a file the client had just read via chunks - T1..Tn applied twice. The advance-only entry read now runs on every chunk pass, moving the cursor to the true disk content end before memory is consulted. Ordering inside the pass is load-bearing: the entry read can outrun the shipped refs by a chunk appended between collection and read, and the transition timestamp becomes the client's refs filter - stamping it past unshipped content would silently drop that chunk's events on the next delta. The pass therefore re-ships the delta after the entry read, so the transition never exceeds shipped content. Bump-displaced aggregated entries can still arrive inline above the cursor with originals below it; that duplication is bounded and stays within the documented at-least-once residual. * fix(filer): prune ref state at the minute the scan actually stops at The collector compares file names at minute granularity while the prune used the exact-nanosecond scan bound, so for a cursor at 12:31:20 the 12-30 file was still collected but its sent state was already deleted - the next pass reshipped the whole file, re-creating the duplicate-refs class the state exists to prevent. Truncate the bound to the minute the file names live in. * fix(filer): derive the chunk cursor from the shipped refs themselves The advance-only entry read left the three positions that must agree in each other's blind spots. Its snapshot could trail the second delta's, so a chunk appended between them shipped events newer than the cursor and the memory pass sent them again. And it made the filer decode the tail range on every pass, serialized ahead of the client's own reads by the transition marker - re-introducing a slice of the replay work chunk mode exists to offload. Compute the cursor from the shipped set instead: the final entry timestamp of each filer's last shipped chunk, decoded once through the shared chunk cache. Refs coverage, transition marker, and memory start are then the same number by construction - nothing is decoded twice, nothing is dropped, and the per-pass server cost falls to one cached chunk decode per filer. The second delta and the once-per-position refs gate existed to patch the entry read's snapshot races, so both go with it; the range read survives only as a fallback for legacy chunks that do not decode standalone. * fix(filer): keep the chunk-cursor probe inside the shipped snapshot Three holes in the tail probe, all variations of stepping outside what was shipped. A permanently missing chunk failed the stream before the transition marker, so the client discarded its pending refs and reconnected to the same failure forever - blocking all later metadata behind one dead volume, where every other replay path (including the client's own reader) skips such chunks; the probe now walks back to the last readable chunk, and a filer with nothing readable simply contributes no cursor. The legacy fallback re-listed the logs after the refs were collected, so a concurrent append could push the range end over an unshipped chunk and the marker past events the client never received; it now streams the shipped chunk list itself, so no snapshot other than the shipped one is ever consulted. And a file selected before UntilNs can hold entries past it, which the client filters while still adopting the marker as its checkpoint - a later bounded request then skipped them; the marker is clamped to the bound. * fix(filer): make the cursor probe an exact mirror of the client's reader The probe answered from the server's view of the chunks; the marker's correctness depends on the client's. Its backward walk found the last readable chunk, but the client reads forward and stops at the first unreadable one, never resuming within a file - for readable, missing, readable the marker claimed the suffix the client never applied, losing those entries permanently. Keeping only each filer's final file ref discarded the progress of earlier readable files when that file was wholly missing, rewinding the marker to the start cursor. And a torn trailing size prefix - what a crashed writer leaves - failed the probe where the client reads a clean end, blocking the marker forever on data the client accepts. The probe is now shaped like the reader it answers for: per file the readable prefix, per filer the newest file with content, and no condition escapes as an error - understating the marker only re-ships, overstating loses events, and a probe failure must never block the transition the client is waiting on. Each rule is pinned by a test that fails against the previous shape. * fix(filer): judge chunk readability at the volumes, not the decode cache Two ways the probe's answer could drift from what the client experiences. A chunk this server decoded earlier stays warm in the shared cache after its volume dies, so the probe sailed past a chunk the direct-reading client stops at - marker beyond the unread suffix, entries lost. Every chunk now passes a volume lookup before the cache is consulted; the lookup rides the master client's in-memory map, so the probe stays cheap. And a probe stop was treated as harmless understatement, but the delta had already marked the whole ref sent: a transient server-side failure left the cursor stranded behind shipped content for the life of the connection, parking aggregated streams below the watermark for data the client already holds. The pass now rolls back the sent state of every ref above the file that answered the probe, so unreached refs re-ship and re-probe until the cursor gets there. Re-shipped entries at or below the client's checkpoint are filtered client-side, and batches are marker-separated, so a re-shipped file cannot rewind a merge mid-batch. * test(filer): end-to-end subscribe-loop harness and wire-contract tests Every escaped bug across this change's review rounds lived in an interaction the unit tests could not see: the loop state machine, the disk/memory handoff, or the server/client contract. The harness runs the real SubscribeLocalMetadata loop against a real leveldb-backed filer, faking only the volume layer behind the existing test hooks, and asserts the delivered stream itself. Eight scenarios, each pinning a class this change was reviewed for: the headline evicted-unflushed gap parks and then delivers in full; a ring that evicted nothing serves memory promptly; a backlog-to-live handoff with 1ms-adjacent timestamps across every boundary delivers exactly once; a flush-proven gap over vacuumed log files skips to the retained ring including a single-entry window; a bounded subscription terminates at its bound; a permanently wedged flush ends in the give-up skip with the stream still alive; and chunk mode is checked against the real client code - pb.ReadLogFileRefs applied to the shipped refs must cover everything the transition marker claims, with and without a dead volume in the middle. Validated by re-introducing three fixed bugs: the missing eviction guard delivers during the unproven gap, a 2ms cursor error at the handoff drops exactly one event, and resuming at rather than below the earliest retained window loses a single-entry window's sole event - each caught by the scenario built for it. The gap timing knobs become vars so parks run at test speed, a small filer hook swaps the volume-touching read functions, and a sender test pins the refs wire rules the client depends on: never batched, never an envelope, everything in order. * fix(filer): re-ship a partially read answering file, pin the probe's limits The sent-state rollback stopped at files newer than the one that answered the probe. When the answering file itself was only prefix-readable - a dead or transient chunk mid-file - its unread suffix stayed marked sent, and the next append advanced the cursor past it for good. The probe now reports whether the answering file was read through to its end, and a prefix-limited answer re-ships that file too; a torn tail counts as complete, since the client's read ends there as well. The rollback rules live in one predicate with a table test - files below a complete answer stay sent, because the client has moved past them and re-shipping cannot rewind its filter. Two test honesty fixes ride along. The loop harness derived its timestamp base from time.Now() per call, so expectations recomputed across a second boundary drifted by exactly one second; the base is now fixed per harness. And the probe's liveness boundary is pinned as a test instead of a comment: a volume lookup cannot see a dead needle or a stale location inside a resolvable volume, so a warm cache can answer past a chunk the client fails on - accepted because metadata log chunks die volume-at-a-time and the alternative is a real read per probe, which is what the probe exists to avoid. The test states the boundary so changing it is a decision, not an accident. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-08-02 00:06:22 -07:00
Jaehoon KimandClaude Fable 5	c17aebeecf	fix(filer.backup): prevent silent backup data loss on transient not-found (#10295) ... * fix(filer.backup): stop silently dropping events on transient not-found Under a write burst, filer.backup could consume a metadata event (advancing the persisted offset) without replicating the file, with no error logged: 1. filersink CreateEntry/UpdateEntry swallowed replicateChunks errors (glog.Warningf + return nil), so the offset advanced past entries that were never written. 2. The manifest-chunk branch of replicateChunks resolved via LookupFileId with no retry, unlike the data-chunk branch — transient lookup races dropped exactly the large manifest-backed files while small inline-content siblings landed. 3. isIgnorable404 matched "LookupFileId" / "volume id ... not found", misclassifying those races as genuine source 404s at the backup layer. Fix: on a replicateChunks failure the filer sink now skips only when the live source has moved past the replayed version (deleted or strictly-newer mtime) — lossless, a later event carries the current content — and propagates otherwise so the event is retried. The manifest resolve retries transient errors like the data-chunk path. isIgnorable404 is narrowed to genuine 404s; non-filer sinks and the initial-snapshot walk, which relied on the broad match as their only lossless-skip valve, now make the same live-source decision (filersink.SourceSupersedes) instead of retrying forever on a permanently gone volume. Tests cover propagation of unconfirmed lookup failures, the narrowed 404 classification, and the supersession guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer.backup): derive supersession path with directory fallback eventSourceSuperseded built the source path from NewParentPath alone. Legacy metadata events (persisted by older filers) carry an empty NewParentPath, so the probe looked up "/<name>", read the miss as "source gone", and skipped a live file on a transient lookup error — the silent drop this change is meant to eliminate. Derive the path via MetadataEventTargetFullPath (the same directory fallback genProcessFunction uses) and cover both event shapes with TestEventSupersessionProbe_PathDerivation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filersink): retry manifest resolve only for transient errors, bounded when unverifiable The manifest-resolve retry stopped only when hasSourceNewerVersion proved the source moved past the replayed version, which wedged the sink in two cases: incremental sinks use dated target keys that cannot map back to a source path (supersession never provable), and permanent resolve errors (corrupt manifest data, bad file ids) fail forever while the source entry stays live. Gate the retry instead: keep retrying only transient errors (volume-lookup races, network interruptions), stop after a few attempts when supersession cannot be checked, and propagate everything else immediately so the configured metadata error policy applies (-disableErrorRetry included). Propagation is lossless: filer.backup's fallback decides with the event's real source key, and both filer.backup and filer.sync re-deliver the event (RetryForeverOnError) without advancing the offset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filer.backup): make error classifiers nil-safe isIgnorable404, isSourceLookupError, and isTransientResolveError called err.Error() without a nil guard. All current call sites pass a non-nil error, but the guard is free and matches isRetryableNetworkError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-07-10 01:47:46 -07:00
Jaehoon KimandChris Lu	ebeab4b6ec	feat(filer.sync.verify): reclassify chunk-slice-order ETag diffs as CHUNK_REORDER (#10177) ... * feat(filer.sync.verify): reclassify chunk-slice-order ETag diffs as CHUNK_REORDER filer.ETagChunks concatenates per-chunk MD5s in stored slice order without normalising by offset, so byte-identical content written by two different paths (S3 multipart part-completion order on the source vs filer.backup replication arrival order on the destination) yields different file ETags. filer.sync.verify reported these as ETAG_MISMATCH even though the files are equal. Add a second-pass check on every ETAG_MISMATCH: when both sides derive their ETag from chunks (no attr.Md5) and hold a manifest-free, non-overlapping chunk set that, once sorted by offset, matches element-wise on (offset, size, ETag), classify the file as CHUNK_REORDER. Such files are content-equal, so they are not counted as errors and do not affect the exit code; they are listed only at higher verbosity (weed -v=1), while the summary always shows their count. The check stays conservative: a stored attr.Md5 (order-independent content hash), a differing chunk count, an overlapping/duplicate offset (whose visible bytes are resolved by timestamp), or a manifest chunk all remain ETAG_MISMATCH. * filer.sync.verify: decline chunk-reorder fast path on empty per-chunk ETag An empty or undecodable per-chunk ETag is not a content fingerprint, so element-wise (offset, size, ETag) equality can't prove the bytes match. Treating "" == "" as content-equal could reclassify a genuine divergence as CHUNK_REORDER and drop it from the error count. Decline such chunk sets so they stay ETAG_MISMATCH. * filer.sync.verify: emit CHUNK_REORDER in JSON output regardless of -v The -v=1 gate belongs to the human text report only. Applying it before the jsonOutput branch dropped the per-file CHUNK_REORDER records from NDJSON while the summary still counted them, so a machine consumer saw a non-zero count with no records to reconcile it. Gate the text path only; JSON always emits. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-07-01 10:38:21 -07:00
Jaehoon KimandClaude Opus 4.8	a11d81b21f	fix(filer.backup): repair chunk-incomplete and stale destination entries (#10082) ... * fix(filer.backup): repair chunk-incomplete and stale destination entries filer.backup left destinations diverged while metadata advanced — chunk-incomplete (missing/gapped ranges at full attr.file_size) or holding a chunk superseded by a missed overwrite. The skip/repair decision keyed on filer.FileSize (the attr), which a truncated entry keeps full, so it never repaired. Decide from actual chunk state instead: - coversReference: range-by-range containment (scalar byte totals and attr FileSize/Md5 cannot see chunk-level gaps). - hasStaleBackupChunk: a backup-written chunk (SourceFileId) the source no longer lists; ignores out-of-band (rsync/direct) chunks. - destinationMatchesReference: allocation-free positional fast path gating the above so they run only on divergence (the in-sync path stays cheap). - A strictly-newer destination is never repaired, so an older out-of-order replay cannot roll it back. The stale signal is deferred at equal mtime (same-second versions cannot be ordered; reliable S3 sub-second ordering is a separate fix). Tests in filer_sink_test.go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * filer.backup: verify chunk range in destinationMatchesReference fast path The allocation-free fast path matched a destination chunk to its reference by SourceFileId alone. That is correct today only because replicateOneChunk copies the source chunk's Offset/Size verbatim, so SourceFileId identity implies an identical range — an invariant that lives in another file with no guard linking the two. If replication ever re-chunks (split/coalesce), a chunk with the right SourceFileId but a different range would fast-path as a full match and skip a needed repair (a false positive in the very class this change otherwise prevents). Compare Offset/Size alongside SourceFileId so the fast path is self-contained and can only be more conservative (a range mismatch falls through to the precise coversReference/hasStaleBackupChunk checks). Add tests for a shifted offset and a larger size at matching identity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-24 14:23:38 -07:00
3 people	20e4614fc6	feat(mount): attach Content-MD5 to chunk uploads (#10016) ... * mount: attach Content-MD5 to chunk uploads Mount writes never set UploadOption.Md5, so FileChunk.ETag stays empty and filer.ETag() degenerates to md5("")-N: same-size files compare equal regardless of content, defeating metadata-level verification like filer.sync.verify. Compute each chunk's MD5 and send it as Content-MD5. The volume server verifies it on ingest (rejecting in-flight corruption) and echoes it back, persisting FileChunk.ETag like filer/S3 writes already do. The dirty-page flush paths already pass a *util.BytesReader whose backing slice is the whole chunk, so the digest is taken in place with no extra read, copy, or allocation (UploadWithRetry unwraps it the same way downstream). Only the rarer plain-reader callers (e.g. manifest chunks) fall back to io.ReadAll. Skipped under -cipher, where only the ciphertext reaches the server. The digest encoding (std-base64 of the raw md5) is the contract the volume server verifies against, so it is factored into contentMD5Base64 and covered by a unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * mount: compute chunk Content-MD5 in the uploader, not the caller Move the WantMd5 hashing into UploadWithRetry, where the chunk is already buffered for the retry path, so saveDataAsChunk stops type-switching the reader and re-reading plain readers. One materialization point, and the cipher exclusion lives next to the hash instead of at every call site. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-06-19 10:29:28 -07:00
3 people	18868e5204	fix(mount): run entry invalidations off the meta-cache apply loop (#10002) ... * fix(mount): run entry invalidations off the meta-cache apply loop The apply loop ran invalidateFunc inline, which acquires the open file handle's lock in fhLockTable. Meanwhile flushMetadataToFiler holds that same fh lock and then waits on the apply loop (applyLocalMetadataEvent). When both target the same open file concurrently, the loop blocks on the fh lock while the lock holder blocks on the loop: an ABBA deadlock that backs up every later readdir/flush and hangs the mount. Fix: dispatch entry invalidations to a dedicated FIFO worker goroutine so the apply loop never blocks on locks held by goroutines waiting on it. Adds a regression test reproducing the interleaving. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * perf(mount): update invalidate counter once per batch Run the batch's invalidateFunc calls without re-taking invalidateMu per item, then bump invalidateProcessed and broadcast once after the loop. WaitForEntryInvalidations only needs the count to reach its target and a batch always completes together, so the per-item lock + broadcast was wasted work. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * mount: extract the invalidate worker into util.AsyncBatchWorker The apply loop's off-thread entry-invalidation queue was a one-off mutex + cond + slice + counters living inside MetaCache. Pull it out as a generic unbounded FIFO worker so the deadlock-avoidance contract (never block the producer, drain on shutdown, wait-for-quiesce) lives in one place. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-06-19 09:19:35 -07:00
Jaehoon KimandChris Lu	202517c02a	fix(filer.backup): skip replay events whose source chunk was superseded or deleted (#9886) ... * fix(filer.backup): skip replay events whose chunk no longer exists on the source "Source" is the filer we replicate FROM (e.g. green in a green->blue backup). Replaying the metadata log from a checkpoint can hit an event whose chunk was since overwritten/deleted and garbage-collected on the source volume. Fetching it returns 0 bytes (a permanent size mismatch), which the sink propagated to the subscription — so the same offset retried forever and replication stalled. Skip the event only when proven stale; otherwise keep refusing so genuine loss of a live file still halts loudly: - onCorruptChunk centralizes the three errChunkSizeMismatch sites. - getEntryMtimeNs compares mtime at nanosecond precision so same-second rewrites (git's config.lock dance) are ordered correctly. - sourceSupersedes re-reads the entry's current state on the source: gone (ErrNotFound) or a strictly-newer mtime than the replayed version -> skip; any other lookup error keeps the entry. Skipping is lossless: events are full-entry snapshots, so a later event re-carries the current chunks and a delete event reconciles a removed file. * test(filer.backup): cover the superseded-chunk skip decision - TestSourceSupersedes: not-found (sentinel / wrapped / gRPC string) and nil entry -> skip; network error -> keep; source newer -> skip; same/older -> keep. - TestGetEntryMtimeNs: nanosecond precision, same-second ordering, nil safety. - TestOnCorruptChunkRefusesWhenSupersessionUnconfirmed: never skip silently when supersession cannot be confirmed. * fix(filer.backup): don't infer supersession for incremental sinks In incremental mode the sink key carries a date prefix (sinkDir/YYYY-MM-DD/relPath) that cannot be reversed to a real source path, so a source lookup would always be ErrNotFound and wrongly classify a live entry as deleted — skipping it. Make targetPathToSourcePath report "unmappable" in incremental mode; hasSourceNewerVersion already declines to skip when the source path cannot be mapped. Found in code review. Non-incremental sinks (filer.backup green->blue) are unaffected. * refactor(filer.backup): name the mtime param sourceMtimeNs; note ns overflow bound - Rename the threaded sourceMtime parameter to sourceMtimeNs across the internal replicate/fetch helpers so the unit is explicit (it only feeds hasSourceNewerVersion, which compares in nanoseconds). - Document that getEntryMtimeNs's int64 ns arithmetic is safe until ~year 2262. No behavior change. * fix(filer.backup): order same-second versions in the CreateEntry skip and update gates The CreateEntry already-replicated short-circuit and chooseUpdateAction still compared second-grained mtime, so a newer version written within the same second could be skipped as already-replicated or overwritten by an older same-second replay. Route both through getEntryMtimeNs, matching the precision the chunk-replication path already uses. * test(filer.backup): cover same-second update-action ordering * docs(filer.backup): trim verbose comments to terse why * fix(filer.backup): check supersession against the rename's new path For a rename the filer sink updates in place (the delete+create branch is skipped for sink name "filer"), so the corrupt-chunk supersession check queried the pre-rename key. Its source-side ErrNotFound was read as "superseded", silently advancing the checkpoint without applying the rename. Map the incoming entry's new path (newParentPath/newEntry.Name) for both update branches. * fix(filer.backup): detect a deleted source even when the replayed mtime is epoch hasSourceNewerVersion returned early when sourceMtimeNs <= 0, skipping the source lookup, so a deleted entry with mtime 0 (a valid epoch timestamp) never got the gone verdict and wedged on permanent retries. Always look up; gate only the newer-mtime comparison on a valid replayed mtime. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-06-09 08:53:29 -07:00
3 people	1b5f1c1f3b	feat(filer.backup): -initialSnapshot re-seeds a reinitialized destination (#9828) ... * feat(filer.backup): add -resetCheckpoint to force a fresh sync filer.backup resumes from a per-sink offset persisted in the source filer's KV. There was no first-class way to discard that checkpoint and re-run from the beginning short of guessing a large -timeAgo, which also skips -initialSnapshot. Add -resetCheckpoint: before reading the offset, write 0 for this sink so getOffset returns 0, isFreshSync stays true, and -initialSnapshot re-runs a full walk. Effective only when -timeAgo is 0. The flag is cleared after the first successful reset: runFilerBackup retries doFilerBackup forever on error, so leaving it set would re-zero the checkpoint on every retry and never make forward progress after a transient failure. Later retries resume from the persisted checkpoint instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(filer.backup): keep fresh-sync intent when offset read fails after reset After -resetCheckpoint writes offset 0, a transient getOffset read-back error flipped isFreshSync to false, which skipped the -initialSnapshot walk the reset explicitly requested. Track that the reset happened this iteration and, on a getOffset error, preserve isFreshSync=true in that case (the non-reset path keeps treating a read error as "not fresh" to avoid re-walking on transients). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(filer.backup): skip offset read-back on reset instead of tracking a flag Replace the didReset bool by branching: on -resetCheckpoint, clear the offset and start fresh without reading it back (we just wrote 0, so the state is known); otherwise read the offset as before. This drops the redundant getOffset RPC after a reset and removes the read-back error case entirely, so no separate flag is needed to preserve isFreshSync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * filer.backup: -initialSnapshot re-seeds on every start; drop -resetCheckpoint -initialSnapshot now walks the live tree whenever -timeAgo is 0, seeds the destination, and overwrites the saved checkpoint, rather than running only on a fresh sync. That re-seeds a reinitialized destination on its own, so the separate -resetCheckpoint flag is gone. The walk runs once per process: the in-memory flag is cleared after the watermark is persisted, so the retry loop resumes from the persisted checkpoint instead of re-walking on every transient error. A process restart re-walks, so remove the flag once the backup is caught up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-06-07 23:35:53 -07:00
Jaehoon KimandChris Lu	4b23204023	fix(vacuum): writable volume re-notification after worker VACUUM (#9732) ... * fix(vacuum): notify master writable after worker vacuum commit Add Phase 3 (markWritableOne) that walks vacuumTargets and calls VolumeMarkWritable on each replica's volume server, mirroring batchVacuumVolumeCommit's per-replica SetVolumeAvailable. Failures are logged at WARN; the task does not fail because the vacuum itself already succeeded. See upstream seaweedfs#9685. * fix(vacuum): delay Phase 3 to let post-commit heartbeats settle Phase 3's VolumeMarkWritable can race with the volume server's first post-commit heartbeat. SetVolumeWritable adds the vid to writables, but a racing heartbeat whose ReadOnly value changed re-runs EnsureCorrectWritables against the master's per-replica cache, and any replica still cached as ReadOnly=true silently removes the vid again — with no further heartbeat change to trigger another recovery. Sleep 30s after Phase 2 (Commit) so every replica's post-vacuum heartbeat has reached the master before Phase 3 fires. Cancel cleanly on ctx.Done so a shutdown during the wait still exits. * fix(vacuum): reduce post-commit settle from 30s to 10s VolumePulsePeriod is 5s, so 10s (2x) is enough margin for every replica's post-commit heartbeat to reach the master before Phase 3 fires. 30s was overly conservative and made TestVacuumExecutionIntegration hit its 30s context deadline. * fix(vacuum): use flat 1m timeout for VolumeMarkWritable RPC VolumeMarkWritable on the volume server is a metadata operation (reopen idx + flags + master ReadOnly=false heartbeat), independent of volume size. Scaling via vacuumTimeout(time.Minute) gave it tens of minutes — even hours on TB volumes — so a single unresponsive replica could block Phase 3 indefinitely. Use a flat 1m cap. * fix(vacuum): gate post-vacuum mark-writable on commit read-only state Phase 3 force-called VolumeMarkWritable on every replica unconditionally, clearing the read-only flag and persisting ReadOnly=false even for a replica left read-only by an operator, an EIO quarantine, or low disk. That overrode states the master deliberately keeps out of writables; master built-in vacuum gates the same step on the commit's IsReadOnly via SetVolumeAvailable. Capture the VacuumVolumeCommit response and skip Phase 3 when any replica came back read-only, letting it recover on its own ReadOnly=false heartbeat. Drop the 10s post-commit settle sleep: the heartbeat race it guarded needed a replica cached read-only at the master, which the gate now excludes. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-05-29 23:43:24 -07:00
Jaehoon Kim	d00acded8a	fix(vacuum): batch all replicas in a single plugin worker task (#9702) ... * fix(vacuum): batch all replicas in a single plugin worker task The plugin worker vacuum path emitted one TaskDetectionResult per (volume, server) replica, but the dispatcher gates duplicate tasks per volume via ActiveTopology.HasAnyTask. The first replica's task was created and the remaining N-1 replicas were silently dropped, so only one replica per volume was ever vacuumed — leaving the others with all their garbage intact. Mirror the master built-in flow (topology.vacuumOneVolumeId → batchVacuumVolumeCheck/Compact/Commit/Cleanup) by: - aggregating detection metrics by VolumeID so a single task carries every replica in TaskParams.Sources - having VacuumTask accept []string servers (instead of a single string), re-check each replica's garbage ratio at execute time to derive a vacuumTargets subset, and run Compact/Commit/Cleanup against only that subset - updating the dispatcher (plugin_handler.Execute, register.CreateTask) to forward every Sources node to NewVacuumTask * fix(vacuum): run all-replica vacuum in two phases to keep failure atomic The prior implementation iterated Compact → Commit → Cleanup against each replica in sequence. A Compact failure on the second replica left the first one already committed (its active files swapped with the .cp* files), producing replica divergence with no automatic recovery. Split performVacuum into two phases, matching topology.vacuumOneVolumeId: Phase 1 — Compact all targets. If any fails, run VacuumVolumeCleanup on every target to drop the .cpd/.cpx/.cpldb temp files, then abort. No replica has swapped yet, so every replica returns to its original state. Phase 2 — Commit all targets. Best-effort, matching batchVacuumVolumeCommit: per-replica errors are collected and surfaced together. Once any replica has swapped there is no clean rollback, so a partial Phase 2 failure requires operator reconciliation. Adds compactOne / commitOne / cleanupOne / cleanupAll helpers and removes the old performVacuumOne. * fix(vacuum): abort when any replica's garbage check fails The prior check tolerated per-replica RPC errors and only failed the task if every replica errored — partial failures were silently treated as "ineligible" so the responding replicas would still be vacuumed. That produces divergence the moment the unreachable replica comes back: it still carries the original garbage while the others have been compacted. Match topology.batchVacuumVolumeCheck's contract instead — its return value (errCount == 0 && len(vacuumLocationList.list) > 0) gates the whole vacuum on every replica's check succeeding. If any replica is unreachable or its VacuumVolumeCheck RPC errors, abort the task; the volume will be retried on the next detection cycle once the replica is healthy. * fix(vacuum): guard against nil metrics and TaskSource entries Detection's bucket-building loop dereferenced m.VolumeID without checking m for nil. VacuumTask.Validate built sourceSet from params.Sources without checking each entry for nil. Both paths would panic on a malformed protobuf payload that managed to deliver a nil slot. Skip nil entries in both loops — neutral with the existing nil/empty filtering already done in register.CreateTask and plugin_handler.Execute. * test(vacuum): success path no longer calls VacuumVolumeCleanup The plugin worker vacuum is now two-phase (Compact-all → Commit-all, with Cleanup only invoked on Compact failure to roll back .cp* temp files). This matches topology.vacuumOneVolumeId, where batchVacuumVolumeCleanup runs only on the Compact-failure branch. On a successful Commit the temp files do not linger: - CommitCompactVolume renames .cpd → .dat and .cpx → .idx - leveldb needle map renames .cpldb → .ldb (needle_map_leveldb.go) so calling VacuumVolumeCleanup afterwards is a redundant no-op. The prior worker code called it unconditionally and the integration test asserted that — switch the expectation to cleanupCalls == 0 to document the new (and master-aligned) contract.	2026-05-27 11:15:25 -07:00
3 people	675020b342	fix(filer.sync): validate chunk size in FilerSink to prevent 0-byte propagation (#9701) ... * fix(filer.sync): validate chunk size in FilerSink to prevent 0-byte propagation FilerSink.fetchAndWrite previously trusted the source response and the upload result blindly: a 200 OK / Content-Length: 0 reply from a broken source volume was happily uploaded as a 0-byte needle to the destination, and the destination filer metadata was then written with the source chunk size. The result was permanent silent corruption -- ls shows the file at its original size but reads fail with EIO. Add two cheap defenses inside fetchAndWrite: 1. After assembling fullData, compare its length against sourceChunk.Size. 2. After a successful upload, compare uploadResult.Size against sourceChunk.Size. Both checks wrap a new sentinel errChunkSizeMismatch that the retry callback recognizes and refuses to retry -- needle.size=0 on disk is a persistent state, not a transient network error, so the sync should stop loudly on the affected entry instead of looping or, worse, silently propagating it. Tests: * TestValidateReplicatedChunkSize -- table-driven coverage of healthy, legitimately empty, zero-byte read, short read, and truncated upload cases. * TestFetchAndWriteRejectsZeroByteSource -- end-to-end: an httptest source that returns 200 OK with an empty body must cause fetchAndWrite to return errChunkSizeMismatch after exactly one source hit (fail fast, no retry storm). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * filer.sync: bubble size-mismatch past CreateEntry/UpdateEntry Three follow-ups on the chunk-size validation: - Use %w in replicateOneChunk so the errChunkSizeMismatch sentinel survives the wrap and reaches errors.Is callers up the stack. - In FilerSink.CreateEntry/UpdateEntry, surface errChunkSizeMismatch instead of warning-and-nil. Other errors (deleted source chunk, transient network) keep the existing swallow so a hiccup doesn't stall the stream. - Drop validateReplicatedUploadSize: uploadResult.Size is set client-side from the same len(fullData) we already validated pre-upload, so the second check can't fail. Test: scope the RetryWaitTime override to the one test that needs it, add a regression that locks in the errors.Is chain through replicateChunks. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-05-26 20:47:53 -07:00
Jaehoon KimandChris Lu	be451d22b5	feat(filer.sync): add -verifySync mode to filer.sync for cross-cluster file comparison (#9284) ... * Add -verifySync flag to filer.sync for cross-cluster file comparison Add a verification mode to filer.sync that compares entries between two filers without performing actual synchronization. Uses directory-level sorted merge of ListEntries to detect missing files, size mismatches, and ETag mismatches. Supports -isActivePassive for unidirectional check and -modifyTimeAgo to skip recently modified files during sync lag. * Add mtime annotation and JSON output to filer.sync -verifySync Add automatic mtime relation analysis for SIZE_MISMATCH and ETAG_MISMATCH diffs, and an NDJSON output mode for external tooling. mtime classification: - B_NEWER => "late_updates_skip_likely" hint. Surfaces the case where target has a stub entry whose mtime is ahead of source's real file, causing UpdateEntry's mtime guard in filersink to permanently skip the update. - A_NEWER => "sync_lag_or_event_miss" hint. - EQUAL => no hint (chunk-level issue suspected). Text output example: [SIZE_MISMATCH] /path (a=996, b=0, B newer +274d [late-updates skip likely]) Add -verifyJsonOutput flag. When set, emits one JSON object per line (NDJSON) for diffs and a final SUMMARY object, suitable for piping into external diagnostic pipelines. Concurrent writes from the directory worker pool are now serialized via outputMu to keep both text lines and JSON records atomic. * fix(filer.sync): use shared global semaphore in verifySync to bound goroutine explosion Replace the per-call local semaphore in compareDirectory with a single shared semaphore created in runVerifySync. The old per-level semaphore applied a limit of verifySyncConcurrency only within each directory level, allowing effective concurrency to grow as verifySyncConcurrency^depth on deep trees. The shared semaphore is held only for each directory's I/O phase (listEntries + merge) and released before recursing into subdirectories, so a parent never blocks waiting for children to acquire slots — which would deadlock once tree depth exceeds the semaphore capacity. Extract the capacity into a named constant (verifySyncConcurrency = 5) with a comment explaining the memory vs. performance trade-off. Add unit tests: - correctness: missing file, only-in-B, size mismatch, active-passive mode - concurrency bound: peak concurrent listings ≤ verifySyncConcurrency - no-deadlock: binary tree of depth 10 completes within timeout * fix(filer.sync): stream directory entries to prevent OOM on large directories Replace the listEntries helper (which accumulated all entries into a single []filer_pb.Entry slice) with an entryStream type that pages through the directory in the background and forwards entries one at a time through a buffered channel. Memory per directory comparison is now O(channel buffer size = 64) regardless of how many entries the directory contains. Key design points: - entryStream wraps a goroutine + buffered channel with a one-entry lookahead (peek/advance) so the two-pointer sorted merge in compareDirectory can work without buffering any full listing. - A child context (mergeCtx) is passed to both stream goroutines so they are cancelled promptly if compareDirectory returns early (e.g. on error); the ctx.Done() select arm in the callback prevents goroutine leaks when the consumer stops reading. - stream.err is written by the goroutine before close(ch), so it is safe to read after the channel is exhausted (Go memory model: channel close happens-before the zero-value receive). - countMissingRecursive is rewritten to use ReadDirAllEntries with a direct callback, eliminating its own slice allocation. - listEntries is removed; it is no longer called anywhere. * fix(filer.sync): address verifySync review findings Four real bugs found and fixed; one finding already resolved (shared semaphore was introduced in a prior commit). path.Join for child paths (filer_sync_verify.go) fmt.Sprintf("%s/%s", dir, name) produced "//name" when dir was "/". Replace all child-path concatenations with path.Join so root-level walks emit clean paths. cutoffTime check for ONLY_IN_B entries (filer_sync_verify.go) The B-only branch ignored -modifyTimeAgo, so files recently written to B were reported as ONLY_IN_B instead of being skipped. Mirror the A-side mtime guard: skip and increment skippedRecent when the entry is newer than cutoffTime. Summary emitted before error check (filer_sync_verify.go) A filer I/O error mid-walk still caused a SUMMARY record (or text summary) to be printed, making partial runs appear complete. Move the error check to before summary emission; on error, return immediately without printing any summary. Return false on verification failure (filer_sync.go) runVerifySync returned true (exit 0) even when diffs were found or the walk failed. Return false so the main binary sets exit status 1, consistent with how all other commands signal failure. * test(filer.sync): add missing verifySync test coverage Four new tests covering gaps identified during review: TestVerifySyncETagMismatch Verifies that two files with identical size but different Md5 checksums are counted as etagMismatch (not sizeMismatch). Exercises the second branch of compareEntries that was previously untested. TestVerifySyncCutoffTime (4 subtests) A-only recent — recent file skipped (skippedRecent++), not MISSING A-only old — old file reported as MISSING B-only recent — recent file skipped (skippedRecent++), not ONLY_IN_B B-only old — old file reported as ONLY_IN_B The B-only subtests specifically cover the cutoffTime fix added in the previous commit. TestVerifySyncRootPath Regression for the path.Join fix: walks from "/" and verifies that the child directory is reached and compared correctly (the old Sprintf produced "//data" which would silently produce wrong results). Asserts dirCount=2 and fileCount=1 to confirm the full tree is walked. * fix(filer.sync): use os.Exit(2) instead of return false on verify failure return false triggered weed.go's error handler which printed the full command usage — appropriate for invalid arguments, not for a completed verification that found differences. Use os.Exit(2) consistent with the existing pattern in filer_sync.go (lines 251, 293). * refactor(filer.sync.verify): split verify into its own command The verify mode is a one-shot batch operation with a fundamentally different lifecycle from the long-running sync subscriber, and most of filer.sync's flags (replication, metrics port, debug pprof, concurrency, etc.) do not apply to it. Extract it into a sibling command alongside filer.copy/filer.backup/filer.export rather than a flag mode on filer.sync. Also rename modifyTimeAgo to modifiedTimeAgo (grammatical) and drop the verifyJsonOutput prefix to plain jsonOutput now that the verify context is implicit in the command name. * fix(filer.sync.verify): address review comments - Bounded worker pool: cap subdirectory goroutines per level via a jobs channel and min(verifySyncConcurrency, len(subDirs)) workers instead of spawning one goroutine per child. Wide directories no longer park ~2KB per queued goroutine. - Don't gate recursion on a directory's mtime: a fresh child write bumps the parent mtime, but older files inside should still be reported as missing. Always recurse for missing-in-B directories and apply the cutoff per-file inside countMissingRecursive. - Apply -modifiedTimeAgo symmetrically: matched-name files now skip the comparison when EITHER side is recently modified, not just A. This restores lag tolerance when B was just rewritten. Adds tests for both new behaviors and a shared isTooRecent helper. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-04-29 12:33:53 -07:00
Jaehoon KimandChris Lu	96af27a131	feat(shell): add fs.distributeChunks command for even chunk distribution (#9117) ... * feat(shell): add fs.distributeChunks command for even chunk distribution Add a new weed shell command that redistributes a file's chunks evenly across volume server nodes. Supports three distribution modes via -mode flag: - primary: balance chunk ownership across nodes (default) - replica: balance both ownership and replica copies - round-robin: assign chunks by offset order for sequential read optimization (chunk[0]->A, chunk[1]->B, chunk[2]->C, ...) Additional options: - -nodes=N to target specific number of nodes - -apply to execute (dry-run by default) Usage: fs.distributeChunks -path=/buckets/file.dat fs.distributeChunks -path=/buckets/file.dat -mode=round-robin -apply fs.distributeChunks -path=/buckets/file.dat -mode=replica -apply fs.distributeChunks -path=/buckets/file.dat -nodes=5 -apply * fix(shell): improve fs.distributeChunks robustness and code quality - Propagate flag parse errors instead of swallowing them (return err) - Handle nil chunk.Fid by falling back to legacy FileId string parsing - Simplify node membership check using slices.Contains * fix(shell): fix dead round-robin print loop in fs.distributeChunks The loop was computing targetNode with sc.index%totalNodes (original chunk index) instead of the sequential position, and discarding it via _ = targetNode without printing anything. Replace with a correct loop using pos%totalNodes and actually print the first 12 node assignments. * fix(shell): compute replication/collection per-chunk in fs.distributeChunks Previously replication and collection were derived once from chunks[0] and reused for all moves, causing wrong volume placement for chunks belonging to different volumes or collections. Now each chunk looks up its own volumeInfoMap entry immediately before calling operation.Assign. * fix(shell): prefer assignResult.Auth JWT over local signing key in fs.distributeChunks When the master returns an Auth token in the Assign response, use it directly for the upload instead of generating a new JWT from the local viper signing key. Fall back to local key generation only when Auth is empty, matching the pattern used by other upload paths. * fix(shell): add timeout and error handling to delete requests in fs.distributeChunks The delete loop was ignoring http.NewRequest errors and had no timeout, risking a nil-request panic or indefinite block. Replace with http.NewRequestWithContext and a 30s timeout, handle request creation errors by incrementing deleteFailCount, and cancel the context immediately after Do returns. * feat(shell): parallelize chunk moves in fs.distributeChunks using ErrorWaitGroup Sequential chunk moves are a bottleneck for large LLM model files with hundreds or thousands of chunks. Use ErrorWaitGroup with DefaultMaxParallelization (10) to run download/assign/upload concurrently. Guard movedRecords appends, chunk.Fid updates, and writer output with a mutex. Individual chunk failures are non-fatal and logged inline; only successfully moved chunks are included in the metadata update. * fix(shell): try all replica URLs on download in fs.distributeChunks Previously only the first volume server URL was attempted, causing chunk moves to fail if that replica was unreachable. Now iterates through all URLs returned by LookupVolumeServerUrl and stops at the first success. * refactor(shell): apply extract method pattern to fs.distributeChunks Do() was a single ~615-line function. Break it into focused helpers: - lookupFileEntry: filer entry lookup - validateChunks: chunk manifest guard - collectVolumeTopology: master topology query + ownership mapping - buildDistributionCounts: chunk→node mapping and owner/copy tallies - selectActiveNodes: target node selection - printCurrentDistribution: per-node distribution table - planDistribution: mode-switch planning (primary/replica/round-robin) - printRedistributionPlan: before/after plan table - relevantNodes: active-or-occupied node filter Do() is now ~100 lines of orchestration; each helper has a single clear responsibility. * test(shell): add unit tests for fs.distributeChunks algorithms Cover all three distribution modes and supporting helpers: - shortName, relevantNodes - computeOwnerTarget (even/uneven split, inactive node drain) - buildDistributionCounts (normal + nil Fid fallback) - selectActiveNodes (all nodes / limited count) - planOwnerMoves (imbalanced → balanced, already balanced) - planDistribution primary (chunks balanced, no-op when even) - planDistribution round-robin (offset ordering, correct assignment) - planDistribution replica (owner + copy balancing) - printRedistributionPlan (output format) * fix(shell): add 5-minute timeout to chunk downloads in fs.distributeChunks Download requests had no per-request timeout, unlike delete operations which already use 30s. Replace readUrl() calls with inline http.NewRequestWithContext + context.WithTimeout(5m) so a hung volume server cannot block a goroutine indefinitely during redistribution. * fix(shell): remove redundant deleteOldChunks in fs.distributeChunks filer.UpdateEntry already calls deleteChunksIfNotNew internally, which computes the diff between old and new entry chunks and deletes the ones no longer referenced. Our explicit deleteOldChunks was racing with this filer-side cleanup, causing spurious 404 warnings on ~75% of deletes. Remove deleteOldChunks, movedChunkRecord type, and reduce executeChunkMoves return type to (int, error) for the moved count. * fix(shell): handle nil chunk.Fid via chunkVolumeId helper in fs.distributeChunks chunk.Fid.GetVolumeId() silently returns 0 for legacy chunks stored with a FileId string instead of a Fid struct, causing them to be skipped in the replica balancing loop and looked up incorrectly in volumeInfoMap. Introduce chunkVolumeId() that uses Fid when present and falls back to parsing the legacy FileId string, matching the logic in buildDistributionCounts. Apply it in the replica-mode copies loop and in executeChunkMoves' replication/collection lookup. * fix(shell): use already-parsed oldFid for volumeInfoMap lookup in fs.distributeChunks chunkVolumeId(chunk) was being called to look up replication/collection after oldFid had already been parsed and validated. Use oldFid.VolumeId directly to avoid redundant parsing and guarantee the correct volume ID regardless of whether chunk.Fid is nil. * fix(shell): improve correctness and robustness in fs.distributeChunks - Buffer download body before upload so dlCtx timeout only covers the GET request; upload runs with context.Background() via bytes.NewReader - Replace 'before, after := strings.Cut(...)' + '_ = before' with '_' as the first return value directly - Clone copiesCount before replica planner mutates it, keeping the caller's map immutable - Add nil-entry guard after filer LookupEntry to prevent panic on unexpected nil response * feat(shell): support chunk manifests in fs.distributeChunks Large files stored as chunk manifests were previously rejected. Resolve manifests up front via filer.ResolveChunkManifest, redistribute the underlying data chunks, then re-pack through filer.MaybeManifestize before UpdateEntry. The filer's MinusChunks resolves manifests on both sides of the diff, so old manifest and inner data chunks are GC'd automatically. * fix(shell): match master's SaveDataAsChunkFunctionType 5-param signature Master added expectedDataSize uint64; ignore it in shell-side saveAsChunk. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-04-17 21:09:36 -07:00
Jaehoon Kim	d3cea714d0	fix(filer.backup): local sink readonly permission (#8907)	2026-04-03 05:36:56 -07:00
3 people	6cf34f2376	Add -filerExcludePathPattern flag and fix nil panic in -filerExcludeFileName (#8756) ... * Fix filerExcludeFileName to support directory names and path components The original implementation only matched excludeFileName against message.NewEntry.Name, which caused two issues: 1. Nil pointer panic on delete events (NewEntry is nil) 2. Files inside excluded directories were still backed up because the parent directory name was not checked This patch: - Checks all path components in resp.Directory against the regexp - Adds nil guard for message.NewEntry before accessing .Name - Also checks message.OldEntry.Name for rename/delete events Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add -filerExcludePathPattern flag and fix nil panic in filerExcludeFileName Separate concerns between two exclude mechanisms: - filerExcludeFileName: matches entry name only (leaf node) - filerExcludePathPattern (NEW): matches any path component via regexp, so files inside matched directories are also excluded Also fixes nil pointer panic when filerExcludeFileName encounters delete events where NewEntry is nil. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Refactor exclude logic: per-side exclusion for rename events, reduce duplication - Extract isEntryExcluded() to compute exclusion per old/new side, so rename events crossing an exclude boundary are handled as delete + create instead of being entirely skipped - Extract compileExcludePattern() to deduplicate regexp compilation - Replace strings.Split with allocation-free pathContainsMatch() - Check message.NewParentPath (not just resp.Directory) for new side Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Move regexp compilation out of retry loop to fail fast on config errors compileExcludePattern for -filerExcludeFileName and -filerExcludePathPattern are configuration-time validations that will never succeed on retry. Move them to runFilerBackup before the reconnect loop and use glog.Fatalf on failure, so invalid patterns are caught immediately at startup instead of being retried every 1.7 seconds indefinitely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add wildcard matching helpers for path and filename exclusion * Replace regexp exclude patterns with wildcard-based flags, deprecate -filerExcludeFileName Add -filerExcludeFileNames and -filerExcludePathPatterns flags that accept comma-separated wildcard patterns (*, ?) using the existing wildcard library. Mark -filerExcludeFileName as deprecated but keep its regexp behavior. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-03-26 10:04:06 -07:00
Jaehoon KimandChris Lu	f2e7af257d	Fix volume.fsck -forcePurging -reallyDeleteFromVolume to fail fast on filer traversal errors (#8015) ... * Add TraverseBfsWithContext and fix race conditions in error handling - Add TraverseBfsWithContext function to support context cancellation - Fix race condition in doTraverseBfsAndSaving using atomic.Bool and sync.Once - Improve error handling with fail-fast behavior and proper error propagation - Update command_volume_fsck to use error-returning saveFn callback - Enhance error messages in readFilerFileIdFile with detailed context * refactoring * fix error format * atomic * filer_pb: make enqueue return void * shell: simplify fs.meta.save error handling * filer_pb: handle enqueue return value * Revert "atomic" This reverts commit 712648bc35. * shell: refine fs.meta.save logic --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>	2026-01-14 21:37:50 -08:00
Jaehoon Kim	d22e3d3495	Fix uncleanable orphans issue with volume.fsck -forcePurging (#7332) ... - Modified `needle_map_memory.go` to include needles with size=0 during needle map loading - Updated `volume_write.go` to handle size=0 needles in delete operations	2025-10-16 12:21:51 -07:00