Repository navigation
fix(scripts): close connections on the failure path, and make a broken close visible - #189
Merged
Merged
Conversation
…n close visible Same class as #186, still present in eight scripts and the stdio server path: `asyncio.run(main())` with `close_all()` as the NEXT statement, no `try/finally`. An exception in the work call skipped the close, aiosqlite's worker threads are non-daemon, `threading._shutdown` joins them forever, and the process prints a traceback and never exits. Reproduced on the exact shape of these scripts: external `timeout 8` -> exit 124; with `finally` -> exit 1 in one second. Fixed at eleven reachable sites: scripts/warm_ner_cache.py, scripts/warm_embedding_cache.py (module-level), scripts/dedup_core_twins.py::run, scripts/dedup_episode_twins.py::run and ::backfill_transitions, scripts/resolve_conflicts.py::run, scripts/mirror_identity.py::run_manifest and ::main._run, scripts/docs_to_wiki.py::_run, scripts/purge_foreign_rows.py::main, and mcp_server/server.py's stdio branch. The stdio branch matters most: it had NO close at all, so a client closing stdin left the server parked in `threading._shutdown` while its parent waited. Measured with a real worker thread: at the base the process exits 124 in both the normal-return and the raising case; now exit 0 and exit 1 respectively. The close failure is logged, not suppressed. A bare `contextlib.suppress(Exception)` would have made a genuinely broken close invisible on the SUCCESS path — exit 0 where the base exited 1, with nothing in the log. Each site now does `except Exception: logger.warning("close_all() failed during shutdown", exc_info=True)`, so the failure is observable while an in-flight exception still propagates unchanged and no exit code is rewritten to compensate. `mirror_identity` also restores `cm.base_dir` in the `finally`: at the base an exception left the module-global pointing at the last manifest base. Not touched: `_shutdown_handler`'s `os._exit(0)` in `mcp_server/server.py` — it is a deliberate way to avoid this hang on the http+dashboard path, and changing it is a separate decision. Found but left alone (pre-existing, outside this package): the same shape in `scripts/sync_skills.py`, `scripts/verify_autohooks.py` and `scripts/backfill_memory_kind.py`.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Cipher208
added a commit
that referenced
this pull request
Nov 3, 2026
… I shipped (#190) 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. Co-authored-by: Cipher208 <269750686+Cipher208@users.noreply.github.com>
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.
Что
Тот же класс, что в #186, остался в восьми скриптах и в stdio-пути сервера:
asyncio.run(main()), аclose_all()— следующей инструкцией, безtry/finally. Исключение в рабочем вызове пропускает закрытие; воркеры aiosqlite не-daemon,threading._shutdownджойнит их вечно, процесс печатает traceback и не завершается.Воспроизведено на точной форме этих скриптов: внешний
timeout 8→ exit 124; сfinally→ exit 1 за секунду.Одиннадцать мест
scripts/warm_ner_cache.pyscripts/warm_embedding_cache.pyscripts/dedup_core_twins.pyrun()scripts/dedup_episode_twins.pyrun()иbackfill_transitions()scripts/resolve_conflicts.pyrun()(включаяassert)scripts/mirror_identity.pyrun_manifest()иmain._run()scripts/docs_to_wiki.py_run()scripts/purge_foreign_rows.pymain()mcp_server/server.pybackfill_transitions— десятое место, найденное verifier'ом на первом круге: оно достижимо из CLI и падало с exit 124.Stdio — худший случай
mcp.run(transport="stdio")вообще не имел закрытия. Клиент закрывает stdin → сервер остаётся вthreading._shutdown, родитель ждёт вечно.mcp.runвернулся (EOF)mcp.runбросилСломанное закрытие теперь видно
Первая версия использовала
contextlib.suppress(Exception)— и verifier доказал, что так настоящий сбой закрытия на успешном пути исчезает: exit 0 там, где база давала 1, и ни строчки в логе.Теперь в каждом месте:
Исходное исключение по-прежнему уходит неизменным, коды возврата не подменяются.
Попутно
mirror_identityвосстанавливаетcm.base_dirвfinally— на базе исключение оставляло глобальную переменную указывать на последнюю базу манифеста.Вне объёма
_shutdown_handlerсos._exit(0)вserver.pyне тронут — это осознанный обход того же зависания для http+dashboard, и его изменение отдельное решение.Найдено, но не тронуто: та же форма в
scripts/sync_skills.py,scripts/verify_autohooks.py,scripts/backfill_memory_kind.py.Проверка
backfill_transitionsи маскировку сбоя закрытия. Второй: всё закрыто, D1 воспроизведён до/после (124 → 1), лог на success-path подтверждён;git diff -w: +108/−26, остальное — переиндентация подtry.