Skip to content

Commit e25c1c0

Browse files
tusharmathlaststylebender14autofix-ci[bot]
authored
fix: add FIXME comments for EMFILE risk in unbounded parallel file reads (#2482)
Co-authored-by: laststylebender14 <ran.mahadik14@gmail.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: laststylebender <43403528+laststylebender14@users.noreply.github.com>
1 parent a437d86 commit e25c1c0

7 files changed

Lines changed: 90 additions & 61 deletions

File tree

‎crates/forge_app/src/changed_files.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ impl<S: FsReadService + EnvironmentService> ChangedFiles<S> {
2626
/// duplicate notifications.
2727
pub async fn update_file_stats(&self, mut conversation: Conversation) -> Conversation {
2828
use crate::file_tracking::FileChangeDetector;
29-
let changes = FileChangeDetector::new(self.services.clone())
29+
let parallel_file_reads = self.services.get_environment().parallel_file_reads;
30+
let changes = FileChangeDetector::new(self.services.clone(), parallel_file_reads)
3031
.detect(&conversation.metrics)
3132
.await;
3233

‎crates/forge_app/src/file_tracking.rs‎

Lines changed: 42 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::sync::Arc;
22

33
use forge_domain::Metrics;
4+
use futures::StreamExt;
45
use tracing::debug;
56

67
use crate::FsReadService;
@@ -17,6 +18,7 @@ pub struct FileChange {
1718
#[derive(Clone)]
1819
pub struct FileChangeDetector<F> {
1920
fs_read_service: Arc<F>,
21+
parallel_file_reads: usize,
2022
}
2123

2224
impl<F: FsReadService> FileChangeDetector<F> {
@@ -25,8 +27,9 @@ impl<F: FsReadService> FileChangeDetector<F> {
2527
/// # Arguments
2628
///
2729
/// * `fs_read_service` - The file system read service implementation
28-
pub fn new(fs_read_service: Arc<F>) -> Self {
29-
Self { fs_read_service }
30+
/// * `parallel_file_reads` - Maximum number of files to hash concurrently
31+
pub fn new(fs_read_service: Arc<F>, parallel_file_reads: usize) -> Self {
32+
Self { fs_read_service, parallel_file_reads }
3033
}
3134

3235
/// Detects files that have changed since the last notification
@@ -39,20 +42,34 @@ impl<F: FsReadService> FileChangeDetector<F> {
3942
/// * `tracked_files` - Map of file paths to their last known hashes (None
4043
/// if unreadable)
4144
pub async fn detect(&self, metrics: &Metrics) -> Vec<FileChange> {
42-
let futures: Vec<_> = metrics
45+
let fs = self.fs_read_service.clone();
46+
// Collect into owned data upfront so the stream futures are 'static-safe
47+
let entries: Vec<(std::path::PathBuf, Option<String>)> = metrics
4348
.file_operations
4449
.iter()
4550
.map(|(path, file_metrics)| {
46-
let file_path = std::path::PathBuf::from(path);
47-
let last_hash = file_metrics.content_hash.clone();
51+
(
52+
std::path::PathBuf::from(path),
53+
file_metrics.content_hash.clone(),
54+
)
55+
})
56+
.collect();
57+
58+
let mut changes: Vec<FileChange> = futures::stream::iter(entries)
59+
.map(|(file_path, last_hash)| {
60+
let fs = fs.clone();
4861

4962
async move {
5063
// Get current hash from the full raw file content (not the
5164
// truncated/formatted content returned to the LLM).
5265
// ReadOutput.content_hash is always computed from the
5366
// unprocessed file, so it is directly comparable with the
5467
// stored hash.
55-
let current_hash = self.read_file_hash(&file_path).await.ok();
68+
let current_hash = fs
69+
.read(file_path.to_string_lossy().to_string(), None, None)
70+
.await
71+
.ok()
72+
.map(|o| o.content_hash);
5673

5774
// Check if hash has changed
5875
if current_hash != last_hash {
@@ -68,30 +85,16 @@ impl<F: FsReadService> FileChangeDetector<F> {
6885
}
6986
}
7087
})
71-
.collect();
72-
73-
let mut changes: Vec<FileChange> = futures::future::join_all(futures)
74-
.await
75-
.into_iter()
76-
.flatten()
77-
.collect();
88+
.buffer_unordered(self.parallel_file_reads)
89+
.filter_map(std::future::ready)
90+
.collect()
91+
.await;
7892

7993
// Sort by path for deterministic ordering
8094
changes.sort_by(|a, b| a.path.cmp(&b.path));
8195

8296
changes
8397
}
84-
85-
/// Reads a file and returns its content hash computed from the full raw
86-
/// content, bypassing any line truncation or range limiting.
87-
async fn read_file_hash(&self, path: &std::path::Path) -> anyhow::Result<String> {
88-
let output = self
89-
.fs_read_service
90-
.read(path.to_string_lossy().to_string(), None, None)
91-
.await?;
92-
93-
Ok(output.content_hash)
94-
}
9598
}
9699

97100
#[cfg(test)]
@@ -198,7 +201,7 @@ mod tests {
198201
let content_hash = compute_hash(content);
199202

200203
let fs = MockFsReadService::new().with_file("/test/file.txt", content);
201-
let detector = FileChangeDetector::new(Arc::new(fs));
204+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
202205

203206
let mut metrics = Metrics::default();
204207
metrics.file_operations.insert(
@@ -219,7 +222,7 @@ mod tests {
219222
let new_hash = compute_hash(new_content);
220223

221224
let fs = MockFsReadService::new().with_file("/test/file.txt", new_content);
222-
let detector = FileChangeDetector::new(Arc::new(fs));
225+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
223226

224227
let mut metrics = Metrics::default();
225228
metrics.file_operations.insert(
@@ -241,7 +244,7 @@ mod tests {
241244
let old_hash = compute_hash("old content");
242245

243246
let fs = MockFsReadService::new().with_not_found("/test/file.txt");
244-
let detector = FileChangeDetector::new(Arc::new(fs));
247+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
245248

246249
let mut metrics = Metrics::default();
247250
metrics.file_operations.insert(
@@ -265,7 +268,7 @@ mod tests {
265268
let old_hash = "old_hash".to_string();
266269

267270
let fs = MockFsReadService::new().with_file("/test/file.txt", new_content);
268-
let detector = FileChangeDetector::new(Arc::new(fs));
271+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
269272

270273
// First call: detect change
271274
let mut metrics = Metrics::default();
@@ -296,7 +299,7 @@ mod tests {
296299
let content_hash = compute_hash(content);
297300

298301
let fs = MockFsReadService::new().with_file("/test/file.txt", content);
299-
let detector = FileChangeDetector::new(Arc::new(fs));
302+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
300303

301304
let mut metrics = Metrics::default();
302305
metrics.file_operations.insert(
@@ -324,7 +327,7 @@ mod tests {
324327
&raw_content,
325328
&displayed_content,
326329
);
327-
let detector = FileChangeDetector::new(Arc::new(fs));
330+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
328331

329332
let mut metrics = Metrics::default();
330333
metrics.file_operations.insert(
@@ -353,7 +356,7 @@ mod tests {
353356
&raw_content,
354357
&displayed_content,
355358
);
356-
let detector = FileChangeDetector::new(Arc::new(fs));
359+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
357360

358361
let mut metrics = Metrics::default();
359362
metrics.file_operations.insert(
@@ -378,7 +381,7 @@ mod tests {
378381
let written_hash = compute_hash(written);
379382

380383
let fs = MockFsReadService::new().with_file("/test/file.txt", written);
381-
let detector = FileChangeDetector::new(Arc::new(fs));
384+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
382385

383386
// Step 1: Read the file (insert via Metrics::insert like production)
384387
let metrics = Metrics::default().insert(
@@ -409,7 +412,7 @@ mod tests {
409412

410413
// Disk now has the externally modified content
411414
let fs = MockFsReadService::new().with_file("/test/file.txt", external);
412-
let detector = FileChangeDetector::new(Arc::new(fs));
415+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
413416

414417
// Step 1: Read, Step 2: Write
415418
let metrics = Metrics::default()
@@ -441,7 +444,7 @@ mod tests {
441444
let content_hash = compute_hash(content);
442445

443446
let fs = MockFsReadService::new().with_file("/test/file.txt", content);
444-
let detector = FileChangeDetector::new(Arc::new(fs));
447+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
445448

446449
// Step 1: Write, Step 2: Read back (overwrites Write entry)
447450
let metrics = Metrics::default()
@@ -481,7 +484,7 @@ mod tests {
481484
.with_file("/test/b.txt", b_external) // user modified B
482485
.with_file("/test/c.txt", c_content)
483486
.with_file("/test/d.txt", d_content);
484-
let detector = FileChangeDetector::new(Arc::new(fs));
487+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
485488

486489
let metrics = Metrics::default()
487490
.insert(
@@ -527,7 +530,7 @@ mod tests {
527530
let modified = "someone changed this";
528531

529532
let fs = MockFsReadService::new().with_file("/test/file.txt", modified);
530-
let detector = FileChangeDetector::new(Arc::new(fs));
533+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
531534

532535
let metrics = Metrics::default().insert(
533536
"/test/file.txt".to_string(),
@@ -551,7 +554,7 @@ mod tests {
551554
let final_hash = compute_hash(final_content);
552555

553556
let fs = MockFsReadService::new().with_file("/test/file.txt", final_content);
554-
let detector = FileChangeDetector::new(Arc::new(fs));
557+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
555558

556559
let metrics = Metrics::default()
557560
.insert(
@@ -585,7 +588,7 @@ mod tests {
585588
let original_hash = compute_hash(original);
586589

587590
let fs = MockFsReadService::new().with_file("/test/file.txt", original);
588-
let detector = FileChangeDetector::new(Arc::new(fs));
591+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
589592

590593
let metrics = Metrics::default()
591594
.insert(
@@ -617,7 +620,7 @@ mod tests {
617620

618621
// After write, disk has the written content
619622
let fs = MockFsReadService::new().with_file("/test/file.txt", written_content);
620-
let detector = FileChangeDetector::new(Arc::new(fs));
623+
let detector = FileChangeDetector::new(Arc::new(fs), 64);
621624

622625
// Read (truncated display), then Write
623626
let metrics = Metrics::default()

‎crates/forge_app/src/orch_spec/orch_setup.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ impl Default for TestContext {
9494
max_image_size: 262144,
9595
workspace_server_url: Url::parse("http://localhost:8080").unwrap(),
9696
max_extensions: 15,
97+
parallel_file_reads: 64,
9798
},
9899
title: Some("test-conversation".into()),
99100
agent: Agent::new(

‎crates/forge_domain/src/env.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,10 @@ pub struct Environment {
9696
/// Set to "json" (or "true"/"1"/"yes") for JSON, "html" for HTML, or
9797
/// unset/other to disable.
9898
pub auto_dump: Option<AutoDumpFormat>,
99+
/// Maximum number of files read concurrently in parallel operations.
100+
/// Controlled by FORGE_PARALLEL_FILE_READS environment variable.
101+
/// Caps the `buffer_unordered` concurrency to avoid EMFILE errors.
102+
pub parallel_file_reads: usize,
99103
}
100104

101105
/// The output format used when auto-dumping a conversation on task completion.
@@ -334,6 +338,7 @@ fn test_command_path() {
334338
workspace_server_url: "http://localhost:8080".parse().unwrap(),
335339
max_extensions: 15,
336340
auto_dump: None,
341+
parallel_file_reads: 64,
337342
};
338343

339344
let actual = fixture.command_path();
@@ -375,6 +380,7 @@ fn test_command_cwd_path() {
375380
workspace_server_url: "http://localhost:8080".parse().unwrap(),
376381
max_extensions: 15,
377382
auto_dump: None,
383+
parallel_file_reads: 64,
378384
};
379385

380386
let actual = fixture.command_cwd_path();
@@ -416,6 +422,7 @@ fn test_command_cwd_path_independent_from_command_path() {
416422
workspace_server_url: "http://localhost:8080".parse().unwrap(),
417423
max_extensions: 15,
418424
auto_dump: None,
425+
parallel_file_reads: 64,
419426
};
420427

421428
let command_path = fixture.command_path();

‎crates/forge_infra/src/env.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,13 @@ impl ForgeEnvironmentInfra {
9797
.unwrap_or_else(|| Url::parse("https://api.forgecode.dev/").unwrap()),
9898
max_extensions: parse_env::<usize>("FORGE_MAX_EXTENSIONS").unwrap_or(15),
9999
auto_dump: parse_env::<AutoDumpFormat>("FORGE_AUTO_DUMP"),
100+
parallel_file_reads: parse_env::<usize>("FORGE_PARALLEL_FILE_READS").unwrap_or_else(
101+
|| {
102+
std::thread::available_parallelism()
103+
.map(|n| n.get() * 2)
104+
.unwrap_or(32)
105+
},
106+
),
100107
}
101108
}
102109

‎crates/forge_infra/src/forge_infra.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ impl ForgeInfra {
6464
let http_service = Arc::new(ForgeHttpInfra::new(env.clone(), file_write_service.clone()));
6565
let file_read_service = Arc::new(ForgeFileReadService::new());
6666
let file_meta_service = Arc::new(ForgeFileMetaService);
67-
let directory_reader_service = Arc::new(ForgeDirectoryReaderService);
67+
let directory_reader_service =
68+
Arc::new(ForgeDirectoryReaderService::new(env.parallel_file_reads));
6869
let grpc_client = Arc::new(ForgeGrpcClient::new(env.workspace_server_url.clone()));
6970
let output_printer = Arc::new(StdConsoleWriter::default());
7071

0 commit comments

Comments
 (0)