mirror of
https://github.com/rustdesk/hbb_common.git
synced 2026-08-27 12:39:50 +00:00
fix: cap the log file by size, and keep LogThrottle usable after poisoning
Rotating on age alone let a single day's file grow without limit, so whoever can drive a hot log site decided how much disk this uses and no amount of per-site throttling could bound it. Add a size criterion, which covers every call site at once — including ones no throttle was added to. LogThrottle: recover the guard on a poisoned lock rather than returning None. Poisoning only means another thread panicked while holding it; the guarded data is two counters that are still usable, and going silent for the rest of the process is worse than a stale count. AGENTS.md permits handling lock poisoning directly, and it forbids swallowing the error. Keep map_or over clippy's is_none_or: that was stabilized in Rust 1.82 and CI pins 1.75. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
This commit is contained in:
+5
-1
@@ -458,7 +458,11 @@ pub fn init_log(_is_async: bool, _name: &str) -> Option<flexi_logger::LoggerHand
|
|||||||
})
|
})
|
||||||
.format(opt_format)
|
.format(opt_format)
|
||||||
.rotate(
|
.rotate(
|
||||||
Criterion::Age(Age::Day),
|
// Size as well as age: rotating only daily lets one day's file grow
|
||||||
|
// without limit, so whoever can drive a hot log site — a peer sending
|
||||||
|
// malformed packets, a socket erroring in a retry loop — decides how
|
||||||
|
// much disk this uses. Bounding it here covers every call site at once.
|
||||||
|
Criterion::AgeOrSize(Age::Day, 16 * 1024 * 1024),
|
||||||
Naming::Timestamps,
|
Naming::Timestamps,
|
||||||
Cleanup::KeepLogFiles(31),
|
Cleanup::KeepLogFiles(31),
|
||||||
)
|
)
|
||||||
|
|||||||
+25
-5
@@ -45,12 +45,17 @@ impl LogThrottle {
|
|||||||
/// The first occurrence after a quiet period always reports, so an isolated fault is not
|
/// The first occurrence after a quiet period always reports, so an isolated fault is not
|
||||||
/// delayed by the interval.
|
/// delayed by the interval.
|
||||||
pub fn due(&self) -> Option<u64> {
|
pub fn due(&self) -> Option<u64> {
|
||||||
let Ok(mut state) = self.state.lock() else {
|
// A poisoned lock only means some other thread panicked while holding it; the guarded
|
||||||
// A poisoned mutex means another thread panicked mid-update; the count is not worth
|
// data is two plain counters that are still usable, and going silent for the rest of
|
||||||
// propagating that, and staying silent is better than logging per call.
|
// the process would be worse than a stale count.
|
||||||
return None;
|
let mut state = self
|
||||||
};
|
.state
|
||||||
|
.lock()
|
||||||
|
.unwrap_or_else(|poisoned| poisoned.into_inner());
|
||||||
state.suppressed += 1;
|
state.suppressed += 1;
|
||||||
|
// `map_or(true, ..)` rather than clippy's preferred `is_none_or`: that was stabilized in
|
||||||
|
// Rust 1.82 and this crate builds on the 1.75 pinned by CI.
|
||||||
|
#[allow(clippy::unnecessary_map_or)]
|
||||||
let due = state
|
let due = state
|
||||||
.last
|
.last
|
||||||
.map_or(true, |last| last.elapsed() >= self.interval);
|
.map_or(true, |last| last.elapsed() >= self.interval);
|
||||||
@@ -66,6 +71,21 @@ impl LogThrottle {
|
|||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
|
// Two directions of one socket need two throttles: an ICMP error on a connected socket is
|
||||||
|
// reported once and cleared, so the steady state alternates (send succeeds, the next recv
|
||||||
|
// reports it) and anything shared between them is reset by the succeeding side every cycle.
|
||||||
|
#[test]
|
||||||
|
fn separate_throttles_do_not_reset_each_other() {
|
||||||
|
let send = LogThrottle::new(Duration::from_secs(60));
|
||||||
|
let recv = LogThrottle::new(Duration::from_secs(60));
|
||||||
|
assert_eq!(recv.due(), Some(1));
|
||||||
|
for _ in 0..1_000 {
|
||||||
|
// The send side succeeding must not hand the recv side a fresh emit slot.
|
||||||
|
assert_eq!(recv.due(), None);
|
||||||
|
}
|
||||||
|
assert_eq!(send.due(), Some(1), "the other direction keeps its own slot");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn first_call_reports_immediately() {
|
fn first_call_reports_immediately() {
|
||||||
let t = LogThrottle::new(Duration::from_secs(60));
|
let t = LogThrottle::new(Duration::from_secs(60));
|
||||||
|
|||||||
Reference in New Issue
Block a user