Skip to content

fix(tests): clean tirith background install noise - #16457

Open
gorovmaria626-hub wants to merge 1 commit into
NousResearch:mainfrom
gorovmaria626-hub:fix/test-warning-tirith-cleanup
Open

fix(tests): clean tirith background install noise#16457
gorovmaria626-hub wants to merge 1 commit into
NousResearch:mainfrom
gorovmaria626-hub:fix/test-warning-tirith-cleanup

Conversation

@gorovmaria626-hub

Copy link
Copy Markdown

Summary

  • Suppress unexpected tirith background installer exceptions so shutdown/closed-stream noise does not leak from daemon threads.
  • Stabilize local interrupt cleanup coverage by capturing the spawned Popen object instead of scraping process command lines.
  • Keep the PR scoped to files still present on current origin/main; legacy RTK integration test was already removed upstream.

Test Plan

  • git diff --check origin/main...HEAD
  • python -m pytest -q tests/tools/test_tirith_security.py tests/tools/test_local_interrupt_cleanup.py tests/tools/test_interrupt.py -W error::pytest.PytestUnhandledThreadExceptionWarning

Review

  • Independent reviewer passed: no security concerns or logic errors.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have tool/terminal Terminal execution and process management labels Apr 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting the installer-thread noise and the flaky process lookup. The current premise remains: ensure_installed() starts _background_install in a daemon thread (tools/tirith_security.py:710-717), and that target directly calls _install_tirith() without an exception boundary (tools/tirith_security.py:621).

Problems

  • Catching Exception would also suppress programming errors, despite the module contract that operational failures are handled while programming errors propagate (tools/tirith_security.py:10-11). It would additionally persist an install-failed marker for those errors.
  • The proposed regression test invokes _background_install() synchronously (PR diff), so it does not validate the thread-warning behavior described in the PR.

Suggested changes

  • Restrict the boundary to the verified operational teardown failure class(es), leaving unexpected implementation errors visible.
  • Exercise the mocked failure through a real background thread and join it before asserting state.

Current main has changed all three touched files since the PR base, including later Tirith work in 07cc567dfa208bc7b1adf59dd1f21243930af319; salvage should reapply the narrow fix against that state.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 12, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Four PRs touch or reference the local interrupt-cleanup area, but they address distinct causes: #16457 combines Tirith installer-thread exception handling with deterministic subprocess capture, #39496 adjusts platform skips and timeout budget, #43157 primarily adds a self-host-kill guard while also retuning interrupt-test timing, and #49460 implements Windows process-tree teardown that has since landed independently on main.

Related pull requests

  • #16457 related — (+49/-39) — keep open, revisions required: capturing the spawned Popen object directly addresses the flaky process-command-line scan, but the Tirith change catches every Exception and its synchronous test does not reproduce daemon-thread warning leakage. The keep_open review on #16457 should be resolved by limiting the boundary to verified teardown failures and exercising it through a real joined background thread.
  • #39496 related — (+6/-0) — keep open, revisions required: the 75-second timeout addresses the collision between the test's 30-second cleanup wait and the file-level timeout, but the module-wide POSIX skip would also suppress the newer Windows tree-kill regression. Per the keep_open review on #39496, rebase and scope skips only to POSIX-dependent tests while leaving the Windows test active.
  • #43157 related — (+398/-13) — do not merge as-is: the interrupt-test budget reduction targets a real timeout race, but most of the diff is a separate self-host-kill feature whose shell-token and shell -c detection currently introduces documented false positives. Despite the keep_open review on #43157, contributor reviews remain blocking until numeric PID matching is retained without treating the terminal shell's $$ as the Python host or scanning data-position shell tokens as executable code.
  • #49460 [closed] related — (+82/-1) — remain closed as implemented on main: it correctly replaced wrapper-only Windows termination with whole-tree teardown and added focused regression coverage, but commit e5253d8 independently landed the same behavior through terminate_pid(..., force=True) and taskkill /T /F. It remains relevant as the original focused reference implementation and explains why Windows coverage must not be skipped by #39496.

Suggested consolidation

Do not merge any PR as-is. Use #16457 as the narrow consolidation target after fixing its exception boundary and threaded regression test; optionally fold in only the rebased, per-test POSIX markers and timeout-budget work from #39496. Keep #43157 separate and blocked on its contributor-reviewed guard false positives, and leave #49460 closed because its Windows tree-kill fix is already implemented on main; none of these PRs is a full duplicate that should be closed solely in favor of #16457.

Cross-PR triage: Reviewed 4 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 35 kB of PR diffs, 7 kB of issue/PR text, 16 kB of discussion (13 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants