Skip to content

Commit f1fd17f

Browse files
committed
fix: Crash if rescan triggered before bigramming is over
1 parent 51e0ef7 commit f1fd17f

4 files changed

Lines changed: 90 additions & 74 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/fff-core/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ bindet = { workspace = true }
3434
blake3 = { workspace = true }
3535
chrono = { workspace = true }
3636
dirs = { workspace = true }
37+
libc = "0.2"
3738
git2 = { workspace = true }
3839
glidesort = { workspace = true }
3940
globset = { workspace = true }

crates/fff-core/src/file_picker.rs

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,15 @@ pub struct FilePicker {
319319
cancelled: Arc<AtomicBool>,
320320
bigram_index: Option<Arc<BigramFilter>>,
321321
bigram_overlay: Option<Arc<parking_lot::RwLock<BigramOverlay>>>,
322+
// This is a soft lock that we use to prevent rescan be triggered while the
323+
// bigram indexing is in progress. This allows to keep some of the unsafe magic
324+
// relying on the immutabillity of the files vec after the index without worrying
325+
// that the vec is going to be dropped before the indexing is finished
326+
//
327+
// In addition to that rescan is likely triggered by something unnecessary
328+
// before the indexing is finished it means that fff is dogfooded the index either
329+
// by the UI rendering preview or simply by walking the directory. Which is not good anyway
330+
post_scan_busy: Arc<AtomicBool>,
322331
}
323332

324333
impl std::fmt::Debug for FilePicker {
@@ -411,6 +420,7 @@ impl FilePicker {
411420
has_explicit_cache_budget: has_explicit_budget,
412421
is_scanning: Arc::new(AtomicBool::new(false)),
413422
mode: options.mode,
423+
post_scan_busy: Arc::new(AtomicBool::new(false)),
414424
scanned_files_count: Arc::new(AtomicUsize::new(0)),
415425
sync_data: FileSync::new(),
416426
warmup_mmap_cache: options.warmup_mmap_cache,
@@ -445,6 +455,7 @@ impl FilePicker {
445455
let watcher_ready = Arc::clone(&picker.watcher_ready);
446456
let synced_files_count = Arc::clone(&picker.scanned_files_count);
447457
let cancelled = Arc::clone(&picker.cancelled);
458+
let post_scan_busy = Arc::clone(&picker.post_scan_busy);
448459
let path = picker.base_path.clone();
449460

450461
{
@@ -463,6 +474,7 @@ impl FilePicker {
463474
shared_picker,
464475
shared_frecency,
465476
cancelled,
477+
post_scan_busy,
466478
);
467479

468480
Ok(())
@@ -908,6 +920,14 @@ impl FilePicker {
908920
return Ok(());
909921
}
910922

923+
// The post-scan warmup + bigram phase holds a raw pointer into the
924+
// current files Vec. Replacing sync_data now would free that memory.
925+
// Skip — the background watcher will retry on the next event.
926+
if self.post_scan_busy.load(Ordering::Acquire) {
927+
debug!("Post-scan bigram build in progress, skipping rescan");
928+
return Ok(());
929+
}
930+
911931
self.is_scanning.store(true, Ordering::Relaxed);
912932
self.scanned_files_count.store(0, Ordering::Relaxed);
913933

@@ -1003,6 +1023,7 @@ fn spawn_scan_and_watcher(
10031023
shared_picker: SharedPicker,
10041024
shared_frecency: SharedFrecency,
10051025
cancelled: Arc<AtomicBool>,
1026+
post_scan_busy: Arc<AtomicBool>,
10061027
) {
10071028
std::thread::spawn(move || {
10081029
// scan_signal is already `true` (set by the caller before spawning)
@@ -1094,6 +1115,7 @@ fn spawn_scan_and_watcher(
10941115
watcher_ready.store(true, Ordering::Release);
10951116

10961117
if warmup_mmap_cache && !cancelled.load(Ordering::Acquire) {
1118+
post_scan_busy.store(true, Ordering::Release);
10971119
let phase_start = std::time::Instant::now();
10981120

10991121
// Scale cache limits based on repo size (skip if caller provided an explicit budget).
@@ -1110,7 +1132,9 @@ fn spawn_scan_and_watcher(
11101132
}
11111133

11121134
// SAFETY: The file index Vec is not resized between the initial scan
1113-
// completing and the warmup + bigram phase finishing.
1135+
// completing and the warmup + bigram phase finishing because
1136+
// `post_scan_busy` prevents concurrent rescans from replacing
1137+
// sync_data while we hold the raw pointer.
11141138
let files_snapshot: Option<(&[FileItem], Arc<ContentCacheBudget>)> =
11151139
if !cancelled.load(Ordering::Acquire) {
11161140
let guard = shared_picker.read().ok();
@@ -1120,7 +1144,9 @@ fn spawn_scan_and_watcher(
11201144
let ptr = files.as_ptr();
11211145
let len = files.len();
11221146
let budget = Arc::clone(&picker.cache_budget);
1123-
// SAFETY: see comment above — Vec is stable during this window.
1147+
// SAFETY: post_scan_busy flag blocks trigger_rescan and
1148+
// background watcher rescans from replacing sync_data,
1149+
// so the Vec backing this slice stays alive.
11241150
let static_files: &[FileItem] =
11251151
unsafe { std::slice::from_raw_parts(ptr, len) };
11261152
(static_files, budget)
@@ -1171,6 +1197,8 @@ fn spawn_scan_and_watcher(
11711197
}
11721198
}
11731199

1200+
post_scan_busy.store(false, Ordering::Release);
1201+
11741202
info!(
11751203
"Post-scan warmup + bigram total: {:.2}s",
11761204
phase_start.elapsed().as_secs_f64(),

crates/fff-core/src/log.rs

Lines changed: 58 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1,93 +1,91 @@
11
//! Shared logging utilities for FFF crates.
22
//!
3-
//! Provides file-based tracing initialization and a panic hook that writes
4-
//! to both stderr and a fallback log file.
3+
//! Provides file-based tracing initialization and crash handlers (panic hook
4+
//! + SIGSEGV signal handler) that write diagnostics to both stderr and the
5+
//! configured log file.
56
67
use std::io;
7-
use std::path::Path;
8+
use std::path::{Path, PathBuf};
89
use tracing_appender::non_blocking;
910
use tracing_subscriber::fmt::format::FmtSpan;
1011
use tracing_subscriber::{EnvFilter, fmt, prelude::*};
1112

1213
static TRACING_INITIALIZED: std::sync::OnceLock<tracing_appender::non_blocking::WorkerGuard> =
1314
std::sync::OnceLock::new();
1415

15-
static PANIC_HOOK_INSTALLED: std::sync::OnceLock<()> = std::sync::OnceLock::new();
16+
static CRASH_HANDLERS_INSTALLED: std::sync::OnceLock<()> = std::sync::OnceLock::new();
1617

17-
/// Install panic hook that writes to both stderr and a fallback file.
18-
/// This is called separately from init_tracing to ensure panics are always logged.
18+
/// The log file path set by `init_tracing`. Crash handlers append to this file.
19+
static LOG_FILE_PATH: std::sync::OnceLock<PathBuf> = std::sync::OnceLock::new();
20+
21+
fn write_crash_report(header: &str, body: &str) {
22+
let msg = format!(
23+
"\n=== CRASH {} ===\n{}\n=== CRASH END {} ===\n",
24+
header, body, header
25+
);
26+
27+
let _ = std::io::Write::write_all(&mut std::io::stderr(), msg.as_bytes());
28+
29+
if let Some(path) = LOG_FILE_PATH.get() {
30+
let _ = std::fs::OpenOptions::new()
31+
.create(true)
32+
.append(true)
33+
.open(path)
34+
.and_then(|mut f| std::io::Write::write_all(&mut f, msg.as_bytes()));
35+
}
36+
}
37+
38+
extern "C" fn sigsegv_handler(sig: libc::c_int) {
39+
let bt = std::backtrace::Backtrace::force_capture();
40+
write_crash_report("SIGSEGV", &format!("signal {}\n{}", sig, bt));
41+
42+
unsafe {
43+
libc::signal(sig, libc::SIG_DFL);
44+
libc::raise(sig);
45+
}
46+
}
47+
48+
/// Install both the panic hook and the SIGSEGV signal handler.
1949
pub fn install_panic_hook() {
20-
PANIC_HOOK_INSTALLED.get_or_init(|| {
50+
CRASH_HANDLERS_INSTALLED.get_or_init(|| {
2151
let default_panic = std::panic::take_hook();
22-
2352
std::panic::set_hook(Box::new(move |panic_info| {
24-
let payload = panic_info.payload();
25-
let message = if let Some(s) = payload.downcast_ref::<&str>() {
53+
let message = if let Some(s) = panic_info.payload().downcast_ref::<&str>() {
2654
s.to_string()
27-
} else if let Some(s) = payload.downcast_ref::<String>() {
55+
} else if let Some(s) = panic_info.payload().downcast_ref::<String>() {
2856
s.clone()
2957
} else {
3058
"Unknown panic payload".to_string()
3159
};
3260

33-
let location = if let Some(location) = panic_info.location() {
34-
format!(
35-
"{}:{}:{}",
36-
location.file(),
37-
location.line(),
38-
location.column()
39-
)
40-
} else {
41-
"unknown location".to_string()
42-
};
61+
let location = panic_info
62+
.location()
63+
.map(|l| format!("{}:{}:{}", l.file(), l.line(), l.column()))
64+
.unwrap_or_else(|| "unknown location".to_string());
4365

44-
// Always log to tracing (if initialized)
4566
tracing::error!(
4667
panic.message = %message,
4768
panic.location = %location,
4869
"PANIC occurred in FFF"
4970
);
5071

51-
// Always print to stderr
52-
eprintln!("=== FFF PANIC ===");
53-
eprintln!("Message: {}", message);
54-
eprintln!("Location: {}", location);
55-
eprintln!("=================");
56-
57-
// Try to write to fallback panic log file
58-
if let Some(cache_dir) = dirs::cache_dir() {
59-
let panic_log = cache_dir.join("fff_panic.log");
60-
let timestamp = std::time::SystemTime::now()
61-
.duration_since(std::time::UNIX_EPOCH)
62-
.map(|d| d.as_secs())
63-
.unwrap_or(0);
64-
65-
let panic_entry = format!(
66-
"\n[{}] PANIC at {}\nMessage: {}\n",
67-
timestamp, location, message
68-
);
69-
70-
let _ = std::fs::OpenOptions::new()
71-
.create(true)
72-
.append(true)
73-
.open(&panic_log)
74-
.and_then(|mut f| {
75-
use std::io::Write;
76-
f.write_all(panic_entry.as_bytes())
77-
});
78-
79-
eprintln!("Panic logged to: {}", panic_log.display());
80-
}
81-
72+
write_crash_report(
73+
"RUST PANIC",
74+
&format!("Message: {}\nLocation: {}", message, location),
75+
);
8276
default_panic(panic_info);
8377
}));
78+
79+
unsafe {
80+
libc::signal(
81+
libc::SIGSEGV,
82+
sigsegv_handler as *const () as libc::sighandler_t,
83+
);
84+
}
8485
});
8586
}
8687

8788
/// Parse a log level string into a `tracing::Level`.
88-
///
89-
/// Accepts "trace", "debug", "info", "warn", "error" (case-insensitive).
90-
/// Returns `tracing::Level::INFO` for unrecognised values.
9189
pub fn parse_log_level(level: Option<&str>) -> tracing::Level {
9290
match level.as_ref().map(|s| s.trim().to_lowercase()).as_deref() {
9391
Some("trace") => tracing::Level::TRACE,
@@ -100,29 +98,19 @@ pub fn parse_log_level(level: Option<&str>) -> tracing::Level {
10098
}
10199

102100
/// Initialize tracing with a single log file.
103-
///
104-
/// Creates the parent directory if it doesn't exist, truncates the log file,
105-
/// and sets up a non-blocking file appender with structured formatting.
106-
///
107-
/// # Arguments
108-
/// * `log_file_path` - Full path to the log file
109-
/// * `log_level` - Log level (trace, debug, info, warn, error)
110-
///
111-
/// # Returns
112-
/// * `Result<String, io::Error>` - Full path to the log file on success
113101
pub fn init_tracing(log_file_path: &str, log_level: Option<&str>) -> Result<String, io::Error> {
114-
// Install panic hook first (does nothing if already installed)
115-
install_panic_hook();
116-
117102
let log_path = Path::new(log_file_path);
118103
if let Some(parent) = log_path.parent() {
119104
std::fs::create_dir_all(parent)?;
120105
}
121106

107+
let _ = LOG_FILE_PATH.set(log_path.to_path_buf());
108+
install_panic_hook();
109+
122110
let file_appender = std::fs::OpenOptions::new()
123111
.create(true)
124112
.write(true)
125-
.truncate(true) // creates a new file on every setup
113+
.truncate(true) // truncates a file on restart (instead of appending)
126114
.open(log_path)?;
127115

128116
let level = parse_log_level(log_level);
@@ -137,8 +125,6 @@ pub fn init_tracing(log_file_path: &str, log_level: Option<&str>) -> Result<Stri
137125
.with_target(true)
138126
.with_thread_ids(false)
139127
.with_thread_names(false)
140-
// .with_file(true)
141-
// .with_line_number(true)
142128
.with_ansi(false)
143129
.with_span_events(FmtSpan::NEW | FmtSpan::CLOSE),
144130
)

0 commit comments

Comments
 (0)