[chris-dare-dev]

Closes #505.

signal.SIGKILL does not exist on Windows, and ingest/textbook_parser.py evaluated it on two paths that both force-kill a MinerU subprocess. Neither killed anything on Windows: both raised AttributeError, the subprocess kept running with its sandbox output directory open, and the failure surfaced as something unrelated to a timeout.

The defect was present twice

Fixing the obvious half would not have worked.

1 — at the call sites (:443 shutdown, :551 wall timeout), signal.SIGKILL is an argument. It is evaluated before the function is entered, so no platform check inside the helper could ever run.

2 — inside the helper itself:

if hasattr(os, "getpgid"):
    os.killpg(os.getpgid(proc.pid), sig)
elif sig == signal.SIGKILL:      # the Windows branch, raising on Windows
    proc.kill()

That elif sits on the branch taken when os.getpgid is absent — which is Windows. The arm written to handle Windows could not execute on Windows. AttributeError is not an OSError, so the enclosing contextlib.suppress did not catch it either.

The helper's docstring already reasoned about exactly this hazard for os.getpgid, one line above where it recurred for signal.SIGKILL.

The fix

The helper takes an intent (force: bool), never a signal number, so the constant is named only inside the POSIX branch where it is guaranteed to exist:

def _signal_process_group(proc: subprocess.Popen, *, force: bool) -> None:
    with contextlib.suppress(ProcessLookupError, OSError):
        if hasattr(os, "getpgid"):
            os.killpg(os.getpgid(proc.pid), signal.SIGKILL if force else signal.SIGTERM)
        elif force:
            proc.kill()
        else:
            proc.terminate()

That makes the Windows arm total by construction rather than by remembering.

Rejected alternative, because it is silently wrong rather than merely uglier: a module-level _SIGKILL = getattr(signal, "SIGKILL", signal.SIGTERM), which #505 floated. On Windows it collapses to SIGTERM, so the comparison would be true for both phases of terminate_live_mineru's SIGTERM → grace → SIGKILL escalation, and the graceful phase would hard-kill.

Documented, not hidden: on Windows force changes nothing, because Popen.terminate and Popen.kill are both TerminateProcess. A caller needing a real grace period on Windows does not get one here. The pre-existing accepted gap is separate and unchanged — on Windows the kill reaches only the direct child, and MinerU's grandchild FastAPI service may survive (security-pdf-sandbox.md, CLAUDE.md gotcha #10).

Why this shipped, and what stops it recurring

The existing test_timeout_falls_back_to_proc_kill_without_getpgid removes os.getpgid only. On POSIX signal.SIGKILL still resolves, so that test passed in CI while failing on Windows. The guard could not see the bug it was written next to.

Two tests added:

• test_timeout_kills_when_the_platform_has_no_sigkill removes both attributes, reproducing Windows on any host. Verified to fail against the pre-fix file with the real AttributeError at :551 — that is what makes it a regression test rather than a restatement of the fix.
• test_signal_helper_never_names_sigkill_off_the_posix_branch drives the helper directly for both intents, so a future refactor of the timeout path cannot quietly drop the coverage. It fails pre-fix on the signature, so it guards the helper going forward rather than re-proving the original defect.

Verification

• tests/test_textbook_parser.py and tests/test_parse_tracker.py pass.
• Ruff clean.
• Revert-proof: reverting ingest/textbook_parser.py to origin/main while keeping the new tests makes all three relevant tests fail.
• The full suite is running as this opens; result will be posted as a comment rather than claimed here.

[chris-dare-dev]

Full suite, as promised when this opened.

5473 passed · 188 skipped · 1 xfailed · 3 failed

Windows 11, CPython 3.11.9, pytest -q -p no:cacheprovider.

tests/test_textbook_parser.py::TestRunMineruSandboxedSurface::test_timeout_falls_back_to_proc_kill_without_getpgid — red on origin/main for this platform — is green, along with both tests added here.

The 3 remaining failures are not this PR's, and not code

FAILED tests/test_desktop_package.py::test_default_needles_cover_raw_and_realpath_temp_root
FAILED tests/test_desktop_package.py::test_verify_cli_exit_codes
FAILED tests/test_desktop_package.py::test_libomp_inventory_counts_files_not_names

All three are WinError 1314: A required privilege is not held by the client — unprivileged symlink creation, which Windows gates behind Developer Mode or an elevated shell. They reproduce identically on a clean origin/main checkout with none of this change applied, and apps/desktop/ is untouched here. Enabling Developer Mode clears them; no code change would.

Where that leaves the platform

For context on how this number was reached, since it moved twice for reasons unrelated to this PR:

	Failures
origin/main on this box	20
after installing mfc (the contract extra, absent from the venv)	4
with this PR	3

The 16-failure drop was ModuleNotFoundError: No module named 'mfc' in test_formal_release_pin.py — an environment gap, not a defect. Worth noting that mfc was installed ad hoc with uv pip install rather than uv sync --extra contract, because on current main any uv sync prunes pytest and ruff out of the project venv. #504 fixes that; until it lands, uv sync is not safe to run in this repository.