From 202c1895e1c2617121f3513054e4a1306d9c573f Mon Sep 17 00:00:00 2001 From: Ayush Kumar Jha <148203331+Ayushraj06-bit@users.noreply.github.com> Date: Sat, 22 Aug 2026 03:29:53 +0530 Subject: [PATCH] fix(wrap): make the Serena pre-index stall budget configurable (#3183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `headroom wrap` blocks the agent launch on a synchronous Serena pre-index whose 300-second ceiling is a hardcoded module constant. When indexing exceeds it the user waits the full five minutes, the work is discarded (`Serena: pre-index timed out (will index on demand)`), and nothing — env var, flag, or config — can shrink that budget. Closes #3093 ### Why this is still open after #2938 `_serena_project_skip_reason` keeps the pre-index off non-project roots, which covers the reporter's two repro directories. But it **defers the stall by one wrap rather than removing it**: as that function's own docstring notes, Serena's MCP server generates `project.yml` itself on first start, "so the pre-index simply resumes from the next wrap onwards." A parent-of-many-repos directory therefore gets claimed during the first session and pays the full 300s budget on every wrap after that. The reporter's remaining ask — "I'd also like the pre-index timeout to be configurable" — is the unfixed half. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - Added `HEADROOM_SERENA_INDEX_TIMEOUT` and `_resolve_serena_index_timeout_seconds()`, modelled on the existing `_resolve_wrap_proxy_timeout_seconds()` in the same module. - `_index_serena_project` resolves the budget after the `uvx` guard and passes it to `communicate()` instead of the bare constant. - `_SERENA_INDEX_TIMEOUT = 300` stays as the default, so unset behavior is unchanged. - Added 19 tests covering the resolver and the pre-index call path. ### Deliberate divergence from the proxy-timeout precedent `_resolve_wrap_proxy_timeout_seconds` raises `RuntimeError` on a bad value, which is right for a subsystem the wrap cannot proceed without. The pre-index is documented as best-effort and non-fatal, so raising there would let a typo'd env var abort a launch that would otherwise succeed. An unusable value instead warns and falls back to 300s. The warning is unconditional (not gated on `--verbose`) because a knob that looks applied but is not is the failure this issue reports. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ python -m pytest tests/test_cli/test_wrap_serena_boost.py -q 43 passed, 1 skipped, 1 warning in 1.13s # 24 pre-existing + 19 new # the same 19 tests against the unpatched tree: 18 failed, 1 passed, 24 deselected # the 1 passer is a pre-existing test caught by -k $ python -m pytest tests/test_cli/ -q 3 failed, 696 passed, 2 skipped in 57.80s # the 3 are pre-existing Windows failures (symlink handling in test_recover_codex.py # and test_unwrap_claude.py); they fail identically on an unpatched tree. $ python -m ruff check headroom/cli/wrap.py tests/test_cli/test_wrap_serena_boost.py All checks passed! $ python -m ruff format --check headroom/cli/wrap.py tests/test_cli/test_wrap_serena_boost.py 2 files already formatted $ python -m mypy headroom/cli/wrap.py Success: no issues found in 1 source file ``` Regression check across all 42 test modules that import `headroom.cli.wrap`, run in both states with the working tree md5-verified before each run: identical 81-line failure/error set, +19 passing with the fix. ## Real Behavior Proof - Environment: Windows 11 Home 26200, Python 3.11.9, headroom at 0.36.2 (`5e0ce24`). Serena/`uvx` are not installed on this machine and the Rust `_core` extension is not built (no Rust toolchain), so a full `headroom wrap claude` could not be launched — see `Not tested`. - Exact command / steps: drove the real `_index_serena_project()` with a real child process, a real process group, real `communicate(timeout=...)`, real `TimeoutExpired`, and the real `_kill_serena_index_tree`, timing each phase with a monotonic clock at `HEADROOM_SERENA_INDEX_TIMEOUT=2` and `=4`. Only *which* binary runs was substituted (a 120s sleeper in place of `serena project index`), since the timeout logic is indifferent to the callee. - Observed result: the configured budget controls the wait exactly — a 2s budget waits 2.02s and a 4s budget waits 4.02s, where before the change the same harness reports 300s regardless of any env var set. Full output below. - Not tested: an end-to-end `headroom wrap claude/opencode` against a real `serena project index` (uvx/serena unavailable here); non-Windows platforms; the interaction with a genuinely large monorepo index. ```text budget=2s | waited 2.02s for timeout | teardown 10.02s | total 12.03s budget=4s | waited 4.02s for timeout | teardown 10.02s | total 14.03s misconfigured value: Serena: ignoring HEADROOM_SERENA_INDEX_TIMEOUT='30s' (want a positive integer number of seconds) - using 300s -> resolved to 300s, no exception raised ``` ### Incidental finding (not addressed here) On Windows, `_kill_serena_index_tree` adds a constant ~10s after any timed-out pre-index — one of its two 10s bounds (`taskkill` / `proc.wait`) is hit every time. So a 2s budget still costs ~12s wall clock. That is pre-existing #2938 code untouched by this PR, but it caps how small the stall can usefully get and may deserve its own issue. ## Runtime Rollout Safety - Rollout-managed feature(s): none — this is a plain env var, not a rollout-channel feature. - Minimum rollout channel: n/a — available on every channel, inert unless set. - Stable/default behavior changed: no — unset resolves to the existing 300s constant. - Kill switch / disable path: unset `HEADROOM_SERENA_INDEX_TIMEOUT`; skipping the pre-index entirely remains `--no-serena`. - Unsafe override required: no. - Qualification impact: none — no change to compression, proxy, or provider behavior. - Rollback path: revert the commit; no persisted state, no migration, no config to clean up. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I did **not** edit `CHANGELOG.md` — it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this) ## Additional Notes **Alternatives considered.** A CLI flag (`--serena-index-timeout`) is more discoverable but has to be threaded through four `wrap` subcommands, adds CLI surface that CONTRIBUTING gates behind maintainer sign-off, and would not reach `wrap ... -- agents` sessions. Making the pre-index asynchronous removes the stall outright and is arguably the better end state, but it is an architectural change and would reopen the orphaned-grandchild failure mode #2938 just closed. Auto-scaling the budget by project size reintroduces the kind of hand-maintained heuristic #2938 deliberately removed. **What this does not solve.** The default is still 300s, so a user who never sets the variable still stalls; the reporter's third point (using Serena in background agent sessions launched from a parent directory) is a Serena-semantics question rather than a headroom defect; and an in-flight pre-index is still not interruptible. **Open questions for maintainers.** 1. Should `0` mean "skip the pre-index" instead of being rejected? I kept the proxy-timeout precedent (reject `<= 0`) since `--no-serena` already covers disabling, but the other reading is defensible. 2. `HEADROOM_WRAP_PROXY_TIMEOUT` — the closest precedent — is not in `docs/content/docs/configuration.mdx`, so I matched it and left docs alone. Happy to add a row if you would rather document it. 3. If you consider a new env knob a feature rather than part of this bug, say so and I will hold for a maintainer sign-off before you spend review time. Documentation: no `CHANGELOG.md` edit (release-please generates it from the PR title). --- headroom/cli/wrap.py | 43 +++++++- tests/test_cli/test_wrap_serena_boost.py | 127 +++++++++++++++++++++++ 2 files changed, 167 insertions(+), 3 deletions(-) diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index 03be77c4c..638d9e56a 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -1816,6 +1816,40 @@ def _serena_project_skip_reason(root: Path) -> str | None: #: Upper bound on the synchronous pre-index. The agent does not launch until #: this call returns, so the number is a stall budget, not just a safety net. _SERENA_INDEX_TIMEOUT = 300 +_SERENA_INDEX_TIMEOUT_ENV = "HEADROOM_SERENA_INDEX_TIMEOUT" + + +def _resolve_serena_index_timeout_seconds() -> int: + """Resolve the Serena pre-index stall budget from env, else the default. + + A wrap launched from a directory Serena has already claimed re-indexes the + whole tree on every run, and 300s of that is time the agent is not running + (#3093). The budget is therefore tunable per environment, which also keeps + it reachable from ``wrap ... -- agents`` sessions that take no flags. + + Unlike :func:`_resolve_wrap_proxy_timeout_seconds`, a bad value is not + fatal here: the pre-index is best-effort, so an unusable setting falls back + to the default rather than aborting a launch that would otherwise succeed. + It is reported unconditionally, because a knob that looks applied but is + not is the failure this issue is about. + """ + raw = os.environ.get(_SERENA_INDEX_TIMEOUT_ENV, "").strip() + if not raw: + return _SERENA_INDEX_TIMEOUT + + timeout_seconds: int | None + try: + timeout_seconds = int(raw) + except ValueError: + timeout_seconds = None + if timeout_seconds is None or timeout_seconds <= 0: + click.echo( + f" Serena: ignoring {_SERENA_INDEX_TIMEOUT_ENV}={raw!r} " + f"(want a positive integer number of seconds) " + f"— using {_SERENA_INDEX_TIMEOUT}s" + ) + return _SERENA_INDEX_TIMEOUT + return timeout_seconds def _kill_serena_index_tree(proc: subprocess.Popen) -> None: @@ -1875,8 +1909,9 @@ def _index_serena_project(*, verbose: bool = False) -> None: so any failure here is survivable. This runs on the launch path, synchronously: the agent starts only once it - returns, so the timeout below is time the user spends staring at nothing. - Two guards keep that bounded (#2938): + returns, so the timeout below is time the user spends staring at nothing — + ``HEADROOM_SERENA_INDEX_TIMEOUT`` resizes that budget (#3093). Two guards + keep it bounded (#2938): * ``stdin`` is ``DEVNULL``. Serena prompts when it has to auto-create ``project.yml``, and because stdout is captured the question never @@ -1892,6 +1927,8 @@ def _index_serena_project(*, verbose: bool = False) -> None: click.echo(" Serena: uvx not found — skipping pre-index") return + timeout_seconds = _resolve_serena_index_timeout_seconds() + popen_kwargs: dict[str, Any] = { "stdout": subprocess.PIPE, "stderr": subprocess.PIPE, @@ -1931,7 +1968,7 @@ def _index_serena_project(*, verbose: bool = False) -> None: # the output is captured, so without this line the wrap looks hung. click.echo(" Serena: pre-indexing project (first run can take a while)…") try: - _stdout, stderr = proc.communicate(timeout=_SERENA_INDEX_TIMEOUT) + _stdout, stderr = proc.communicate(timeout=timeout_seconds) except subprocess.TimeoutExpired: _kill_serena_index_tree(proc) click.echo(" Serena: pre-index timed out (will index on demand)") diff --git a/tests/test_cli/test_wrap_serena_boost.py b/tests/test_cli/test_wrap_serena_boost.py index cc3f4ef81..4901cc1ab 100644 --- a/tests/test_cli/test_wrap_serena_boost.py +++ b/tests/test_cli/test_wrap_serena_boost.py @@ -257,6 +257,133 @@ def test_preindex_spawn_failure_is_non_fatal( wrap_cli._index_serena_project(verbose=True) # must not propagate +# --------------------------------------------------------------------------- +# HEADROOM_SERENA_INDEX_TIMEOUT — the stall budget is tunable (#3093) +# --------------------------------------------------------------------------- + + +def _clear_index_timeout(monkeypatch: pytest.MonkeyPatch) -> None: + """Resolve from a known-empty environment, not the developer's shell.""" + monkeypatch.delenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, raising=False) + + +def test_index_timeout_defaults_when_env_is_unset(monkeypatch: pytest.MonkeyPatch) -> None: + _clear_index_timeout(monkeypatch) + + assert wrap_cli._resolve_serena_index_timeout_seconds() == wrap_cli._SERENA_INDEX_TIMEOUT + + +def test_index_timeout_reads_the_env(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, "45") + + assert wrap_cli._resolve_serena_index_timeout_seconds() == 45 + + +@pytest.mark.parametrize("raw", [" 30 ", "\t30\n"]) +def test_index_timeout_tolerates_surrounding_whitespace( + raw: str, monkeypatch: pytest.MonkeyPatch +) -> None: + """An env var exported from a shell heredoc keeps its padding.""" + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, raw) + + assert wrap_cli._resolve_serena_index_timeout_seconds() == 30 + + +@pytest.mark.parametrize("raw", ["", " "]) +def test_index_timeout_treats_a_blank_value_as_unset( + raw: str, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """``export HEADROOM_SERENA_INDEX_TIMEOUT=`` is not a misconfiguration.""" + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, raw) + + assert wrap_cli._resolve_serena_index_timeout_seconds() == wrap_cli._SERENA_INDEX_TIMEOUT + assert capsys.readouterr().out == "" # no warning noise on the default path + + +def test_index_timeout_accepts_the_smallest_useful_budget( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, "1") + + assert wrap_cli._resolve_serena_index_timeout_seconds() == 1 + + +def test_index_timeout_accepts_a_budget_longer_than_the_default( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A deliberately huge monorepo may want *more* than 300s, not less.""" + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, "86400") + + assert wrap_cli._resolve_serena_index_timeout_seconds() == 86400 + + +@pytest.mark.parametrize("raw", ["0", "-1", "abc", "30s", "1.5", "1e3", "0x10", "None"]) +def test_index_timeout_falls_back_on_an_unusable_value( + raw: str, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """Best-effort means degrade to the default, never abort the launch.""" + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, raw) + + assert wrap_cli._resolve_serena_index_timeout_seconds() == wrap_cli._SERENA_INDEX_TIMEOUT + + # A silently-ignored knob is the bug being fixed, so say so — and quote the + # value back, since the usual cause is a unit suffix the parser rejects. + out = capsys.readouterr().out + assert wrap_cli._SERENA_INDEX_TIMEOUT_ENV in out + assert repr(raw) in out + + +def test_index_timeout_survives_a_float_unrepresentable_budget( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``int()`` accepts integers ``float()`` cannot hold; resolving must not raise. + + ``communicate`` would go on to raise ``OverflowError`` adding that to a + monotonic clock, which the caller's generic handler already absorbs as a + non-fatal skip — so the budget is honoured as given rather than clamped. + """ + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, "1" + "0" * 400) + + assert wrap_cli._resolve_serena_index_timeout_seconds() == 10**400 + + +def test_preindex_uses_the_env_budget(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The resolved budget reaches ``communicate`` — the point of #3093.""" + monkeypatch.chdir(tmp_path) + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, "5") + _stub_uvx(monkeypatch) + proc = _FakeProc() + _stub_popen(monkeypatch, proc) + seen: list[float | None] = [] + monkeypatch.setattr( + proc, "communicate", lambda timeout=None: (seen.append(timeout), ("", ""))[1] + ) + + wrap_cli._index_serena_project() + + assert seen == [5] + + +def test_preindex_still_runs_on_an_unusable_budget( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A bad value must not skip the pre-index or propagate out of it.""" + monkeypatch.chdir(tmp_path) + monkeypatch.setenv(wrap_cli._SERENA_INDEX_TIMEOUT_ENV, "not-a-number") + _stub_uvx(monkeypatch) + proc = _FakeProc() + mock_popen = _stub_popen(monkeypatch, proc) + seen: list[float | None] = [] + monkeypatch.setattr( + proc, "communicate", lambda timeout=None: (seen.append(timeout), ("", ""))[1] + ) + + wrap_cli._index_serena_project() # must not propagate + + mock_popen.assert_called_once() + assert seen == [wrap_cli._SERENA_INDEX_TIMEOUT] + + # --------------------------------------------------------------------------- # _kill_serena_index_tree — no orphaned `serena project index` on timeout # ---------------------------------------------------------------------------