fix(minqlx): propagate the whole minqlx build to existing instances on restart

[dngrtech]

Problem

EVENT_DISPATCHERS["damage"] never registers on existing instances (qlds-27960–27963).

The obvious theory — that the host builds vanilla MinQLX without the damage patch — is wrong. setup_host.yml has applied ql-assets/patches/*.patch under set -e since #145 (2026-07-10), and the build is pinned to SHA fbdd915. The patch is complete and correct.

The bug is propagation, not the build.

Root cause

The damage patch spans two halves, and only one reaches existing instances:

Half	Files	Lives in	Synced on restart?
C	hooks.c, python_dispatchers.c, python_embed.c	minqlx.x64.so	Yes
Python	_events.py (add_dispatcher(DamageDispatcher)), _handlers.py (register_handler("damage", …))	minqlx/ package	No

sync_instance_configs_and_restart.yml — the playbook behind every instance restart — cherry-picked exactly two files out of /home/ql/minqlx-shared/: the .so and the launcher. It never synced the minqlx/ Python package.

So existing instances run a patched binary against a stale Python package. At runtime G_Damage is hooked and the C DamageDispatcher does fire — then it finds damage_handler NULL (the stale _handlers.py never calls register_handler("damage", …)) and returns early. EVENT_DISPATCHERS["damage"] raises KeyError because the stale _events.py never registered the dispatcher.

Newly created instances were always fine: add_qlds_instance.yml copies the whole minqlx-shared/. Only pre-existing instances were affected.

This also means _restart_running_instances — whose docstring read "Used after a minqlx rebuild so instances load the new binary" — has been half-working since it was written. It loaded the binary and left the package stale. That sentence is likely why this survived so long.

Changes

• sync_instance_configs_and_restart.yml — mirror the whole minqlx-shared/ instead of cherry-picking two files, matching what add_qlds_instance.yml already does. Restart and create now converge on identical logic, so no third path can drift. Adds the minqlx_shared_dir var the other three minqlx-aware playbooks already define.
• standalone_host_setup.py — call _restart_running_instances on rerun, mirroring ansible_host_setup.py:183. Standalone hosts previously rebuilt minqlx-shared on rerun and never restarted anything, so the fix would never have landed there.
• HostActionsMenu.jsx — re-run setup now unconditionally restarts running instances, so the modal says so plainly instead of hedging.
• docs/technical.md — the playbook's entry never mentioned minqlx at all, which is part of why this gap went unnoticed.
• common.py — correct the docstring that encoded the false assumption.

Tests

5 new tests, all verified to fail against the pre-fix code:

• test_playbook_defines_minqlx_shared_dir_var
• test_playbook_mirrors_whole_minqlx_shared_dir
• test_minqlx_is_never_cherry_picked_by_file — regression guard
• test_minqlx_sync_precedes_service_restart — the mirror is inert if it ever runs after the restart; nothing enforced this before
• test_standalone_rerun_restarts_running_instances

Full suite: 1029 passing.

Known, deliberate

• copy merges rather than deletes (converge, not byte-identity) — matches add_qlds_instance.yml; synchronize --delete was rejected as riskier against a live instance dir. Inert today (current patches only add files); a future patch that renames/deletes under minqlx/ would leave an orphan in already-provisioned instances.
• lan_rate_enabled instances restart twice on rerun (once via hook migration, once via the minqlx restart). Pre-existing; end state is correct — the second restart is the one that syncs minqlx. Out of scope.

Verification after merge

Re-run Host Setup per QLDS host, then per port 27960–27963:

grep -c 'add_dispatcher(DamageDispatcher)' /home/ql/qlds-<port>/minqlx/_events.py

Expected 1. Pre-fix this is 0.

[github-actions]

PR Review

Summary

This PR fixes a bug where sync_instance_configs_and_restart.yml only copied minqlx.x64.so and run_server_x64_minqlx.sh but not the minqlx/ Python package, leaving patched event dispatchers (e.g. damage) unregistered on existing instances after a minqlx rebuild. The fix replaces the per-file loop with a whole-directory mirror of minqlx-shared/. A standalone-rerun path is also wired to restart running instances after the migration step.

---

Strengths

• Root cause is correctly identified and fixed. The comment in the playbook (silently drops patched event dispatchers) precisely describes the failure mode, which helps future maintainers understand why a whole-directory mirror is required. (ansible/playbooks/sync_instance_configs_and_restart.yml:134-140)
• Regression guard is thorough. test_minqlx_is_never_cherry_picked_by_file checks not just the mirror task but also that no other copy task in the playbook re-introduces per-file cherry-picking. (tests/test_sync_configs_playbook_hooks.py:80-97)
• Ordering test prevents a subtle race. test_minqlx_sync_precedes_service_restart enforces that the sync runs before the restart, which is exactly the invariant that makes the fix meaningful. (tests/test_sync_configs_playbook_hooks.py:100-111)
• ORM-safety comment in the new test. The seen dict workaround for DetachedInstanceError is explained inline, saving the next reader from confusion. (tests/test_rerun_host_setup_migration.py:274-278)
• Documentation is accurate and complete. docs/technical.md now names all three artefacts that are mirrored and explains why whole-directory mirroring is necessary.

---

Issues

Critical (Must Fix)

None.

---

Important (Should Fix)

ansible.builtin.copy with remote_src: yes and a directory source is fragile
ansible/playbooks/sync_instance_configs_and_restart.yml:139-148

ansible.builtin.copy with remote_src: yes supports recursive directory copy, but its behaviour for trailing-slash semantics (copy contents vs. copy the directory itself) differs subtly between Ansible versions and is not as battle-tested as ansible.builtin.synchronize (rsync). The rest of the playbook already uses synchronize for similar tasks. Using synchronize here with rsync_opts: ["--delete"] would also remove stale files from a previous build, which copy will not.

- name: Sync minqlx binary and runtime from shared location
  ansible.builtin.synchronize:
    src: "{{ minqlx_shared_dir }}/"
    dest: "{{ qlds_dir }}/"
    delete: yes         # removes stale build artefacts
    owner: yes
    group: yes
    rsync_opts: ["--chown=ql:ql"]
  delegate_to: "{{ inventory_hostname }}"

This matters because a rebuild that removes a file will leave the old copy in place under copy, potentially causing version skew between binary and package again.

---

Whole-directory mirror could propagate unintended files from minqlx-shared/
ansible/playbooks/sync_instance_configs_and_restart.yml:144

If minqlx-shared/ ever accumulates temporary build artefacts (object files, patch rejects, a Makefile, etc.), they will all be pushed into every instance directory. The old per-file list was explicit about what was expected. At minimum, document or enforce what the directory is expected to contain (e.g. via a test that asserts the shared dir only holds known paths). As a code-level fix this is easier to address with synchronize + an explicit include_filter/exclude_filter, or by having rebuild_minqlx.yml produce only the known outputs into minqlx-shared/.

---

Minor (Nice to Have)

UI confirmation message lost its reassuring detail
frontend-react/src/components/HostActionsMenu.jsx:69

The old message told operators that instances "may briefly restart" and "will reconnect automatically". The new message says "Redis and running instances will restart", which is accurate but drops the nuance that reconnection is automatic and that the restart is brief. Users who see "instances will restart" with no qualifier may cancel unnecessarily or expect manual intervention. Suggested wording:

This will re-apply host configuration. Redis and all running instances
will restart briefly; they reconnect automatically.

---

Triple-nested with in the new test can be flattened
tests/test_rerun_host_setup_migration.py:286-298

Python 3 allows multiple context managers on a single with statement. Three levels of nesting here make the indentation deeper than necessary:

with (
    patch("ui.task_logic.common._restart_running_instances", side_effect=_record_restart) as restart_running,
    patch("ui.task_logic.ansible_instance_hooks.apply_instance_hooks_logic", return_value=True),
    app.app_context(),
):
    ...

---

mode: preserve silently inherits whatever permissions the build leaves behind
ansible/playbooks/sync_instance_configs_and_restart.yml:148

The previous task set mode: '0755' explicitly on both files. mode: preserve is correct for a whole-directory mirror, but if a build ever produces a file without the execute bit (e.g. a .py file that shouldn't need it but the binary does), there is no enforcement. This is unlikely to cause a real problem but worth a comment noting it is intentional.

---

Assessment

Ready to merge? Yes, with the "Important" items tracked.

Reasoning: The bug fix is correct and well-tested; the regression guards are thorough. The ansible.builtin.copy/synchronize issue is real but unlikely to cause immediate production failures — it warrants a follow-up issue rather than blocking this fix.