Skip to content

cleanup: improve error message when inode map is full #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Molter73 merged 1 commit into from
May 28, 2026
Merged

cleanup: improve error message when inode map is full #729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fact/src/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ pub struct FactCli {
///
/// Going over this limit will prevent fact from tracking and
/// filling in the host path correctly in events.
#[arg(long, short, env = "FACT_INODE_MAX")]
#[arg(long, short, env = "FACT_INODES_MAX")]
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify that tests exist covering the renamed FACT_INODES_MAX environment variable

# Search for tests referencing the new environment variable name
rg -n "FACT_INODES_MAX" fact/src/config/tests.rs

# Also check for any tests covering inodes_max CLI parsing
rg -nC5 "inodes_max" fact/src/config/tests.rs

Repository: stackrox/fact

Length of output: 4987

Add/adjust unit tests to cover the FACT_INODE_MAX → FACT_INODES_MAX env-var rename.
fact/src/config/tests.rs currently covers parsing/validation of the bpf.inodes_max field from config strings (e.g., inodes_max: 64/16384 and related type/validation cases), but there’s no evidence of tests asserting the environment variable binding for FACT_INODES_MAX (or the old name). Add unit tests that set the env var and verify it populates the inodes_max config value.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fact/src/config/mod.rs` at line 523, Add unit tests that set the
FACT_INODES_MAX environment variable and assert it populates the bpf.inodes_max
config value: in the test module that exercises configuration parsing
(fact/src/config/tests.rs) create a test (e.g., test_env_var_inodes_max) which
uses std::env::set_var("FACT_INODES_MAX", "<value>"), invokes the same config
parsing/constructor used in existing tests (the Config builder/parse function
that produces the struct with bpf.inodes_max), then assert config.bpf.inodes_max
== <value>. Also include a test for the old name if desired and ensure tests
clear/reset the env var after running to avoid cross-test pollution.

Copy link
Copy Markdown
Collaborator Author

@Molter73 Molter73 May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai there are currently no unit tests for environment variables, they pose a few issues with how Rust execute tests (all tests run under the same process on parallel threads, so setting environment variables could mess with the tests in unforeseen ways). I can try to figure out a way to test this properly, but I suggest we leave that for a follow up PR.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Molter73, that's a fair point — std::env::set_var is process-global and inherently racy when tests run in parallel, making it tricky to test env-var bindings reliably without extra infrastructure (e.g., serializing those tests with a mutex or running them with --test-threads=1). Deferring to a follow-up PR makes sense.

Would you like me to open a GitHub issue to track adding env-var coverage for FACT_INODES_MAX (and potentially other env-var bindings) in a future PR?

Copy link
Copy Markdown
Collaborator Author

@Molter73 Molter73 May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai, please do open the issue.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo stackrox/fact \
  --title "test: add unit tests for environment variable bindings (e.g. FACT_INODES_MAX)" \
  --body "## Summary

Add unit tests that verify environment variable bindings correctly populate the corresponding configuration values.

Currently there are no tests covering env-var bindings for CLI arguments such as \`FACT_INODES_MAX\`, \`FACT_INODE_MAX\` (old name), and other env-vars mapped in \`FactCli\`. These were deferred because \`std::env::set_var\` is process-global and inherently racy when tests run in parallel under the same process.

## Motivation

The rename of \`FACT_INODE_MAX\` → \`FACT_INODES_MAX\` in `#729` highlighted the lack of test coverage for env-var bindings. A regression here (wrong env-var name in the \`#[arg(env = ...)]\` attribute) would not be caught by existing tests.

## Suggested approach

- Serialize env-var tests using a shared \`Mutex\` (or the \`serial_test\` crate) so they don't interfere with parallel tests.
- Alternatively, scope tests to run with \`RUST_TEST_THREADS=1\` or via a dedicated test binary target.
- At minimum, add coverage for \`FACT_INODES_MAX\` populating \`bpf.inodes_max\`, and consider covering other env-var bindings in \`FactCli\`.
- Ensure each test cleans up the env var (e.g., via \`std::env::remove_var\`) to prevent cross-test pollution.

## References

- PR: https://github.com/stackrox/fact/pull/729
- Comment: https://github.com/stackrox/fact/pull/729#discussion_r3318634169

/cc `@Molter73`" \
  --assignee Molter73

Length of output: 1633

@Molter73, done! I've opened #730 to track adding env-var unit test coverage (including FACT_INODES_MAX) in a follow-up PR, assigned to you.

inodes_max: Option<u32>,

/// Whether configuration should be hotreloaded
Expand Down
29 changes: 24 additions & 5 deletions fact/src/host_scanner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,18 @@

use std::{
cell::RefCell,
io,
os::linux::fs::MetadataExt,
path::{Path, PathBuf},
sync::Arc,
time::Duration,
};

use anyhow::{Context, bail};
use aya::maps::MapData;
use aya::{
maps::{MapData, MapError},
sys::SyscallError,
};
use fact_ebpf::{inode_key_t, inode_value_t, monitored_t};
use globset::{Glob, GlobSet, GlobSetBuilder};
use log::{debug, info, warn};
Expand Down Expand Up @@ -183,10 +187,25 @@ impl HostScanner {

/// Similar to update_entry except we are are directly using the inode instead of the path.
fn update_entry_with_inode(&self, inode: inode_key_t, path: PathBuf) -> anyhow::Result<()> {
self.kernel_inode_map
.borrow_mut()
.insert(inode, 0, 0)
.with_context(|| format!("Failed to insert kernel entry for {}", path.display()))?;
match self.kernel_inode_map.borrow_mut().insert(inode, 0, 0) {
Ok(_) => {}
Err(MapError::SyscallError(SyscallError { io_error, .. }))
if io_error.kind() == io::ErrorKind::ArgumentListTooLong =>
{
bail!(
r#"Reached maximum number of inodes to track.
You can increase this limit with:
* The bpf.inodes_max configuration value.
* The FACT_INODES_MAX environment variable.
* The --inodes-max argument."#,
)
}
e => {
return e.with_context(|| {
format!("Failed to insert kernel entry for {}", path.display())
});
}
}

let mut inode_map = self.inode_map.borrow_mut();
let entry = inode_map.entry(inode).or_default();
Expand Down
Loading