cleanup: improve error message when inode map is full#729
Conversation
When reaching the maximum number of inodes that can be tracked, attempting to insert a new entry in the kernel map will produce an error message pointing the user to increase the value via: * The bpf.inodes_max configuration value. * The FACT_INODES_MAX environment variable. * The --inodes-max argument. There is also a minor cleanup on the name of the environment variable for consistency among all the configuration options.
📝 WalkthroughWalkthroughA small PR that updates the environment variable binding for inode tracking configuration from ChangesInode Tracking Configuration and Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@fact/src/config/mod.rs`:
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: e1e17999-f511-4442-9d3a-10129c2892eb
📒 Files selected for processing (2)
fact/src/config/mod.rsfact/src/host_scanner.rs
| /// 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")] |
There was a problem hiding this comment.
🧩 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.rsRepository: 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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@coderabbitai, please do open the issue.
There was a problem hiding this comment.
🧩 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 Molter73Length 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.
When reaching the maximum number of inodes that can be tracked, attempting to insert a new entry in the kernel map will produce an error message pointing the user to increase the value via: * The bpf.inodes_max configuration value. * The FACT_INODES_MAX environment variable. * The --inodes-max argument. There is also a minor cleanup on the name of the environment variable for consistency among all the configuration options.
2 participants
Description
When reaching the maximum number of inodes that can be tracked, attempting to insert a new entry in the kernel map will produce an error message pointing the user to increase the value via:
There is also a minor cleanup on the name of the environment variable for consistency among all the configuration options.
This change will need to be backported to the
release-0.3branch.Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Monitored a directory with lots of files and limited the amount to track with
--inodes-max 64.Without the change fact crashes with the message:
With the change:
Summary by CodeRabbit
Chores
FACT_INODES_MAXBug Fixes