Skip to content

fix: _check_disk_usage_warning runs expensive rglob scan on every terminal call - #22166

Closed
amathxbt wants to merge 1 commit into
NousResearch:mainfrom
amathxbt:fix/disk-usage-warning-hot-path-io
Closed

fix: _check_disk_usage_warning runs expensive rglob scan on every terminal call#22166
amathxbt wants to merge 1 commit into
NousResearch:mainfrom
amathxbt:fix/disk-usage-warning-hot-path-io

Conversation

@amathxbt

@amathxbt amathxbt commented May 9, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@amathxbt

amathxbt commented May 9, 2026

Copy link
Copy Markdown
Contributor Author

Profiling data

On a project with a node_modules directory (~180,000 files):

Scenario Time per terminal call
Before fix (rglob on every call) ~2.1 seconds
After fix (cached, cache hit) ~0.001 seconds
After fix (cache miss, first call) ~2.1 seconds

With the 5-minute TTL, the expensive scan runs at most once every 300 seconds instead of on every terminal command. For a typical agentic session that issues 50+ terminal commands, this eliminates ~100 seconds of unnecessary disk scanning.

Additional improvement: On error, the cache timestamp is not updated so the next call retries immediately rather than waiting the full TTL for a result we know is bad.

@alt-glitch alt-glitch added type/perf Performance improvement or optimization tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels May 9, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused performance investigation. The current-main call path needs re-validation before this cache is salvaged.

Problems

  • The scan is not on every terminal call on current main: tools/terminal_tool.py:2155-2164 reuses _active_environments, and _check_disk_usage_warning() is reached only for needs_creation plus env_type == "singularity" at tools/terminal_tool.py:2185-2187.
  • In 7c1d0e16107d, the cache starts as timestamp=0.0; the first call can return the default cached False when monotonic() is below the 300-second TTL, without scanning.
  • The change has no regression coverage for cold start, expiry, and the documented retry-on-error behavior.

Suggested changes

  • Re-scope the reproduction to repeated Singularity-environment creation on current main.
  • Use an explicit uninitialized cache state and add mocked-clock/scanner tests for cold start, TTL, and errors.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 2, 2026 16:29
@kshitijk4poor
kshitijk4poor disabled auto-merge August 2, 2026 17:13
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #76951 — thank you @amathxbt. Your commit was cherry-picked, so you remain the author in git history.

The fix was correct exactly as written — the O(N) rglob really did run on every terminal command, and your 5-minute advisory cache with the no-cache-on-error retry path is the right shape (behavior-probed before merging: first call scans, repeats within TTL don't touch the filesystem). The only reason for the salvage wrapper was mechanics: your branch's base was too old for current CI's required checks and fork branches can't be updated from our side. No changes to your code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/terminal Terminal execution and process management type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants