Repository navigation
fix(scripts): the last three close sites, and stop the test pollution I shipped - #190
Merged
Merged
Conversation
… I shipped Three scripts still had the #189 class, each in a different shape: - `sync_skills.py::sync_one` closed AFTER `sync_external`, so a raise there skipped the close entirely (measured at 2cffe18: close never called, the process hung and was killed at 20 s); - `verify_autohooks.py` closed on the success path only, in the same function that ends in `os._exit`. An exception therefore hung until the 45 s faulthandler guard fired, and the exit code came from the guard rather than from the real failure. The checks moved into `_run_checks()` and `main()` now closes in a `finally` before `os._exit` — which stays where it is, on purpose, and outside the `try`: inside it, `finally` would never run; - `backfill_memory_kind.py` already had `finally`; its defect was a silent `contextlib.suppress(Exception)` around the close, which made a genuinely broken shutdown indistinguishable from a clean one. All three now log a failed close (`logger.warning(..., exc_info=True)`) instead of swallowing it, matching #189. Separately, this fixes a regression #189 put on master. `test_close_all_on_failure.py` patched `shared.write_dispatch.get_write_dispatch`, and `core.memory` — which does `from shared.write_dispatch import get_write_dispatch` at module level — was first imported lazily INSIDE that patched window. It bound the stub permanently, so `test_mirror_identity.py` later failed with the stub's own `RuntimeError: boom`: `pytest tests/test_scripts` read 7 failed on 2cffe18 against 162 passed on b5060fe. The full suite hid it behind test ordering. An autouse fixture now snapshots the real function and rebinds it in every loaded module at teardown. It fixes the mechanism rather than the three instances: any module imported inside the window is repaired, not just `core.memory`. The tests are not weakened — reverting any one of the three script fixes still fails its test, checked for all three.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Что
Последние три скрипта того же класса, что #189, — у каждого свой дефект:
scripts/sync_skills.pysync_external→ raise пропускал закрытие (замер: close не вызван, процесс убит на 20 с)finally+ лог сбоя закрытияscripts/verify_autohooks.pyos._exit→ исключение висело до 45-секундного faulthandler_run_checks()+finallyпередos._exit; выход за <1 сscripts/backfill_memory_kind.pyfinallyбыл, ноcontextlib.suppressглотал сбой закрытияexcept Exception: logger.warning(..., exc_info=True)os._exitвverify_autohooksоставлен и стоит внеtry/finally— внутриfinallyникогда бы не выполнился. Код возврата на успехе не изменился (26/26, exit 0).Регрессия, которую я внесла в #189 — здесь же исправлена
pytest tests/test_scriptsна2cffe18: 7 failed / 166 passed. Наb5060fe: 162 passed. Полный suite это скрывал порядком.Причина:
test_close_all_on_failure.pyпатчитshared.write_dispatch.get_write_dispatch, аcore.memory(у негоfrom shared.write_dispatch import get_write_dispatchна уровне модуля) впервые импортируется внутри патченного окна — и навсегда связывает заглушку. Дальшеtest_mirror_identity.pyпадает на её собственномRuntimeError: boom.monkeypatchвосстанавливалshared.write_dispatch, но не копию вcore.memory. Затронуто было четыре модуля:core.episodic,core.memory,core.session,core.session_quality.Лечение — autouse-фикстура: снимает реальную функцию на setup и на teardown перебиндивает её во всех загруженных модулях. Это лечит механизм, а не три конкретных случая: любой модуль, импортированный в окне, чинится.
Тесты не ослаблены
Откат любого из трёх фиксов роняет свой тест — проверено для всех трёх:
sync_skills.py- close / + fail)backfill_memory_kind.pyverify_autohooks.pyПроверка
pytest tests/test_scripts— 177 passed в обоих порядках файлов и на двух сидахpytest-randomly;Остаточный риск, записанный честно
Фикстура живёт в
test_close_all_on_failure.py, поэтому защищает только этот модуль. Тест в другом месте с тем же паттерном (патч + ленивый импорт) снова протечёт; репозиторныйtests/conftest.pyзащитил бы весь suite. Сейчас такого теста нет.