Skip to content

inspect: allow overriding the shared pool thread count - #207

Merged
ualtinok merged 1 commit into
cortexkit:mainfrom
iceteaSA:inspect-pool-thread-override
Aug 10, 2026
Merged

inspect: allow overriding the shared pool thread count#207
ualtinok merged 1 commit into
cortexkit:mainfrom
iceteaSA:inspect-pool-thread-override

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

The dev knob you specced on #205. Built to your shape:

  • read at pool init in default_pool_size(), parsed as usize, clamp(1, 512)
  • absent or unparseable → existing derivation untouched
  • one comment at the site naming its purpose and stating it isn't a tuning surface
  • not in docs/config — dev knob, same class as AFT_STORM_SCALE and AFT_SEMANTIC_QUIET_WINDOW_MS
  • env var, not a config field

Tests cover override-wins, the floor clamp, the 512 cap, and both unparseable shapes ("wide", "-4") falling through to the derivation. Each assertion was watched fail before being trusted — removing the cap gives left: 100000, right: 512.

Two notes on how the test is built, since the trap here is real:

INSPECT_POOL is a process-global LazyLock, so anything testing through the pool would silently pass once another test touched it first. The tests call default_pool_size() directly instead — the parsing and clamping is the logic worth covering and it's pure.

They take crate::test_env::process_env_lock() before mutating the env, matching tool_path.rs. libtest runs unit tests concurrently in one process, so without that lock these would race any other env-mutating test in the binary. The EnvGuard restores the prior value on drop, including the unset case.

Verification: cargo nextest run -p agent-file-tools --lib 2284 passed, 6 skipped · -E 'test(inspect)' 344 passed · release build clean · cargo fmt --check clean · clippy 18 findings on this branch and 18 on main at 8bd35ff4, so the change adds none.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Allow overriding the inspect shared pool thread count via AFT_INSPECT_POOL_THREADS to reproduce thread-regime issues in #205. Unset or invalid values leave the current derivation unchanged.

  • New Features

    • Read AFT_INSPECT_POOL_THREADS at pool init.
    • Parse as usize; clamp to 1..=512.
    • On absence or parse failure, fall back to available_parallelism() capped at 8.
    • Dev-only knob; not a config field; same class as AFT_STORM_SCALE and AFT_SEMANTIC_QUIET_WINDOW_MS.
  • Refactors

    • Extracted resolve_pool_size() for testable parsing/clamping; added unit tests for overrides, clamping, and invalid fallbacks.

Written for commit dd44bb4. Summary will update on new commits.

Review in cubic

Greptile Summary

The PR adds a bounded, development-only environment override for the inspect shared pool size while preserving the existing derivation for absent or invalid values.

  • Reads AFT_INSPECT_POOL_THREADS when initializing the shared pool.
  • Parses the override as usize and clamps it to 1..=512.
  • Extracts resolution logic for deterministic unit testing of valid, clamped, absent, and malformed values.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/aft/src/inspect/dispatch.rs Adds the bounded pool-width override and focused tests without identifying an eligible follow-up defect.

Reviews (2): Last reviewed commit: "inspect: allow overriding the shared poo..." | Re-trigger Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 1 file

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/aft/src/inspect/dispatch.rs Outdated
Comment thread crates/aft/src/inspect/dispatch.rs Outdated
The inspect pool is process-wide and its width derives from
available_parallelism, so there is no way to reproduce a different thread
regime on one machine. glibc allocates arenas per contending thread, so
pool width changes how allocations interleave -- which is the variable
under investigation in cortexkit#205.

AFT_INSPECT_POOL_THREADS overrides the derivation at pool init: parsed as
usize, clamped to 1..=512, with anything absent or unparseable falling
through to the existing derivation unchanged.

Deliberately undocumented and not a config field -- a dev knob in the
same class as AFT_STORM_SCALE and AFT_SEMANTIC_QUIET_WINDOW_MS, per the
standing rule against low-value config surface.
@iceteaSA
iceteaSA force-pushed the inspect-pool-thread-override branch from 46b0a30 to dd44bb4 Compare August 10, 2026 11:34
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Both cubic findings were valid. Fixed in dd44bb4f.

P2 — the race is real and I verified the path rather than taking it on trust. InspectManager::new()start_dispatch_loop (manager.rs:171) → the INSPECT_POOL LazyLockdefault_pool_size(). There are four InspectManager::new() call sites in manager.rs's own test module, none of which take process_env_lock, and libtest runs them concurrently in the same binary as my test. So my process_env_lock only serialized against other lock-takers — a manager test initializing the pool mid-window would have captured 17 or 512 as the process-wide pool width for every subsequent test. Silent, and it would have looked like flake.

Split the parsing out of the env read:

fn default_pool_size() -> usize {
    resolve_pool_size(std::env::var("AFT_INSPECT_POOL_THREADS").ok().as_deref())
}

fn resolve_pool_size(override_value: Option<&str>) -> usize {}

Tests now call resolve_pool_size with supplied input and touch no process state at all — the EnvGuard and the lock are both gone. Net effect on the diff: 36 lines added instead of 67, and the tested surface is the logic that actually matters.

P3 — renamed to pool_thread_override_wins_and_clamps. The suggestion was right; I'd have taken it as-is, but the P2 fix restructured the test anyway so it landed as part of that. Second test renamed to pool_thread_override_ignores_absent_and_unparseable_values since it now also covers the absent case, and I added "" as a third unparseable shape alongside "wide" and "-4".

Re-verified after the change: --lib 2284 passed / 6 skipped · -E 'test(inspect)' 344 passed · release build clean · cargo fmt --check clean. Red control re-run on the new shape — removing the clamp gives left: 0, right: 1.

@ualtinok
ualtinok merged commit 9b092a8 into cortexkit:main Aug 10, 2026
16 checks passed
@iceteaSA
iceteaSA deleted the inspect-pool-thread-override branch August 10, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants