Skip to content

Commit de6980a

Browse files
committed
fix selfupdate race
1 parent 3fc801a commit de6980a

6 files changed

Lines changed: 118 additions & 10 deletions

File tree

‎install/install.sh‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ if [ "${INSTALL_DIR}" != "${TARGET_DIR}" ]; then
7878
rsync -a --delete --exclude ".venv" --exclude ".git" "${INSTALL_DIR}/" "${TARGET_DIR}/"
7979
fi
8080
chown -R "${SERVICE_USER}:${SERVICE_USER}" "${TARGET_DIR}"
81+
# the orchestrator reads its own git HEAD (self-update); keep that working even when a human owns the checkout
82+
if ! git config --system --get-all safe.directory | grep -qxF "${TARGET_DIR}"; then
83+
git config --system --add safe.directory "${TARGET_DIR}"
84+
fi
8185

8286
echo "==> building orchestrator venv"
8387
if [ ! -d "${TARGET_DIR}/.venv" ]; then

‎robot_orchestrator/selfupdate/manager.py‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@
22
import subprocess
33
import sys
44
import tempfile
5+
import threading
56
import time
67
import uuid
78
from dataclasses import dataclass
89
from pathlib import Path
910
from typing import Callable
1011

12+
from robot_orchestrator.bootlog import logged_run
1113
from robot_orchestrator.config import SelfUpdateConfig
1214
from robot_orchestrator.paths import Paths
1315
from robot_orchestrator.repos import git
@@ -29,16 +31,17 @@ class SelfUpdateStatus:
2931
staged_at: float | None
3032
remote_sha: str | None = None
3133
error: str | None = None
34+
in_progress: bool = False
3235

3336

3437
def _default_build_venv(venv_path: Path, release_dir: Path) -> None:
3538
venv_path.parent.mkdir(parents=True, exist_ok=True)
36-
subprocess.run([sys.executable, "-m", "venv", str(venv_path)], check=True, capture_output=True, text=True)
39+
logged_run([sys.executable, "-m", "venv", str(venv_path)], source="self-update", check=True)
3740
pip = str(venv_pip(venv_path))
3841
requirements = release_dir / "requirements.txt"
3942
if requirements.exists():
40-
subprocess.run([pip, "install", "-r", str(requirements)], check=True, capture_output=True, text=True)
41-
subprocess.run([pip, "install", "-e", str(release_dir)], check=True, capture_output=True, text=True)
43+
logged_run([pip, "install", "-r", str(requirements)], source="self-update", check=True)
44+
logged_run([pip, "install", "-e", str(release_dir)], source="self-update", check=True)
4245

4346

4447
def _default_validate(venv_path: Path, release_dir: Path) -> tuple[bool, str]:
@@ -91,16 +94,28 @@ def __init__(
9194
self.venv_builder = venv_builder or _default_build_venv
9295
self.validator = validator or _default_validate
9396
self.source_root = source_root
97+
self._stage_lock = threading.Lock()
9498

9599
def status(self) -> SelfUpdateStatus:
96100
return SelfUpdateStatus(
97101
current_sha=_resolve_current_sha(self.db, self.source_root),
98102
staged_sha=self.db.get_meta("self_update.staged_sha") or None,
99103
staged_at=float(v) if (v := self.db.get_meta("self_update.staged_at")) else None,
100104
error=self.db.get_meta("self_update.last_error") or None,
105+
in_progress=self._stage_lock.locked(),
101106
)
102107

103108
def check_and_stage(self) -> SelfUpdateStatus:
109+
# a second concurrent run would rebuild (and, on failure, delete) the very same release/venv
110+
# directories the first one is still using, so a busy manager just reports its current status
111+
if not self._stage_lock.acquire(blocking=False):
112+
return self.status()
113+
try:
114+
return self._check_and_stage_locked()
115+
finally:
116+
self._stage_lock.release()
117+
118+
def _check_and_stage_locked(self) -> SelfUpdateStatus:
104119
current_sha = _resolve_current_sha(self.db, self.source_root)
105120
try:
106121
git.ensure_mirror_fresh(self.paths.self_mirror, self.config.url, timeout=self.config.fetch_timeout_s)
@@ -125,6 +140,9 @@ def _stage(self, target_sha: str, current_sha: str | None, superseded_sha: str |
125140
staging = self.paths.self_staging(op_id)
126141
release_dir = self.paths.self_release(target_sha[:12])
127142
venv_path = self.paths.self_venv(target_sha[:12])
143+
# leftovers of an attempt that crashed half-way would make the final rename fail
144+
git.rmtree_safe(release_dir)
145+
git.rmtree_safe(venv_path)
128146
try:
129147
git.clone_no_checkout(self.paths.self_mirror, staging)
130148
git.checkout_detach(staging, target_sha)

‎robot_orchestrator/web/routes_api.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ def get_self_update(claims: dict = Depends(require_auth)):
273273
return {
274274
"enabled": c.settings.self_update.enabled,
275275
"current_sha": status.current_sha, "staged_sha": status.staged_sha,
276-
"staged_at": status.staged_at, "error": status.error,
276+
"staged_at": status.staged_at, "error": status.error, "in_progress": status.in_progress,
277277
}
278278

279279
@app.post("/api/self-update/check")
@@ -284,7 +284,7 @@ async def check_self_update(claims: dict = Depends(require_auth)):
284284
status = await asyncio.to_thread(c.self_update_manager.check_and_stage)
285285
return {
286286
"current_sha": status.current_sha, "staged_sha": status.staged_sha,
287-
"staged_at": status.staged_at, "error": status.error,
287+
"staged_at": status.staged_at, "error": status.error, "in_progress": status.in_progress,
288288
}
289289

290290
@app.post("/api/self-update/apply")

‎robot_orchestrator/web/static/admin/index.html‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ <h3 style="margin-top:0;">Оркестратор</h3>
216216
<div class="card">
217217
<h3 style="margin-top:0;">Самообновление оркестратора</h3>
218218
<div id="self-update-info"></div>
219-
<button class="action secondary" onclick="checkSelfUpdate()">Check now</button>
219+
<button id="btn-self-check" class="action secondary" onclick="checkSelfUpdate()">Check now</button>
220220
<button class="action danger" onclick="applySelfUpdate()">Apply staged update</button>
221221
<div id="self-update-msg" class="msg"></div>
222222
</div>
@@ -642,28 +642,41 @@ <h3 style="margin-top:0;">Самообновление оркестратора<
642642
const res = await api("/api/self-update");
643643
const data = await res.json();
644644
const info = document.getElementById("self-update-info");
645+
const escape = (s) => String(s).replace(/[&<>"']/g, (ch) => ({ "&": "&amp;", "<": "&lt;", ">": "&gt;", '"': "&quot;", "'": "&#39;" }[ch]));
645646
info.innerHTML = `
646647
<p><strong>Включено:</strong> ${data.enabled ? "да" : "нет"}</p>
647648
<p><strong>Текущая версия:</strong> <code>${(data.current_sha || "—").slice(0, 12)}</code></p>
648649
<p><strong>Готова к применению:</strong> <code>${(data.staged_sha || "нет").slice(0, 12)}</code></p>
649-
${data.error ? `<p style="color:#f28b8b;">${data.error}</p>` : ""}`;
650+
${data.in_progress ? `<p style="color:#7fb3e0;">Идёт проверка и сборка обновления — это может занять несколько минут…</p>` : ""}
651+
${data.error ? `<p style="color:#f28b8b;">${escape(data.error)}</p>` : ""}`;
652+
document.getElementById("btn-self-check").disabled = !!data.in_progress;
650653
} catch (e) {
651654
document.getElementById("self-update-info").textContent = String(e);
652655
}
653656
}
654657

655658
async function checkSelfUpdate() {
656659
const msg = document.getElementById("self-update-msg");
657-
msg.textContent = "Проверка...";
660+
const button = document.getElementById("btn-self-check");
661+
button.disabled = true;
662+
msg.textContent = "Проверка и сборка обновления… (не нажимайте повторно)";
663+
msg.className = "msg";
658664
try {
659665
const res = await api("/api/self-update/check", { method: "POST" });
660666
const body = await res.json();
661-
msg.textContent = body.staged_sha ? `Готово к применению: ${body.staged_sha.slice(0, 12)}` : (body.error || "Обновлений нет");
662-
msg.className = body.error ? "msg error" : "msg success";
667+
if (body.in_progress) {
668+
msg.textContent = "Проверка уже выполняется — дождитесь результата";
669+
msg.className = "msg";
670+
} else {
671+
msg.textContent = body.staged_sha ? `Готово к применению: ${body.staged_sha.slice(0, 12)}` : (body.error || "Обновлений нет");
672+
msg.className = body.error ? "msg error" : "msg success";
673+
}
663674
loadSelfUpdate();
664675
} catch (e) {
665676
msg.textContent = String(e);
666677
msg.className = "msg error";
678+
} finally {
679+
button.disabled = false;
667680
}
668681
}
669682

‎tests/test_install_assets.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ def test_logind_never_acts_on_idle():
2929
assert "IdleAction=ignore" in _read("systemd/logind-robot-no-idle.conf")
3030

3131

32+
def test_install_script_trusts_the_checkout_so_self_update_can_read_its_head():
33+
script = _read("install.sh")
34+
35+
assert 'git config --system --add safe.directory "${TARGET_DIR}"' in script
36+
37+
3238
def test_install_script_wires_up_every_no_sleep_measure():
3339
script = _read("install.sh")
3440

‎tests/test_selfupdate_manager.py‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,3 +334,70 @@ def test_gc_keeps_current_and_staged_but_removes_superseded_release(env, tmp_pat
334334
assert paths.self_release(sha2[:12]).exists()
335335
assert paths.self_release(sha3[:12]).exists()
336336
assert any(r.name == "strayjunk12" for r in removed)
337+
338+
339+
def test_concurrent_check_is_rejected_while_a_build_is_running(env, tmp_path):
340+
import threading
341+
342+
paths, db, journal = env
343+
src = tmp_path / "src"
344+
sha1 = _init_repo(src, {"main.py": "print(1)\n"})
345+
db.set_meta("self_update.current_sha", sha1)
346+
(src / "main.py").write_text("print(2)\n", encoding="utf-8")
347+
sha2 = _commit_all(src, "second")
348+
349+
build_started = threading.Event()
350+
release_build = threading.Event()
351+
calls = []
352+
353+
def slow_builder(venv_path: Path, release_dir: Path) -> None:
354+
calls.append(venv_path)
355+
venv_path.mkdir(parents=True, exist_ok=True)
356+
build_started.set()
357+
assert release_build.wait(timeout=10)
358+
359+
config = SelfUpdateConfig(enabled=True, url=str(src), branch="main")
360+
manager = SelfUpdateManager(paths, journal, db, config, venv_builder=slow_builder, validator=_fake_validator())
361+
362+
results = []
363+
first = threading.Thread(target=lambda: results.append(manager.check_and_stage()))
364+
first.start()
365+
assert build_started.wait(timeout=10)
366+
367+
second = manager.check_and_stage()
368+
369+
assert second.in_progress is True
370+
assert second.staged_sha is None
371+
assert manager.status().in_progress is True
372+
assert len(calls) == 1
373+
374+
release_build.set()
375+
first.join(timeout=10)
376+
377+
assert results[0].staged_sha == sha2
378+
assert results[0].error is None
379+
assert manager.status().in_progress is False
380+
assert len(calls) == 1
381+
382+
383+
def test_stale_leftovers_from_a_crashed_attempt_do_not_block_staging(env, tmp_path):
384+
paths, db, journal = env
385+
src = tmp_path / "src"
386+
sha1 = _init_repo(src, {"main.py": "print(1)\n"})
387+
db.set_meta("self_update.current_sha", sha1)
388+
(src / "main.py").write_text("print(2)\n", encoding="utf-8")
389+
sha2 = _commit_all(src, "second")
390+
391+
stale_release = paths.self_release(sha2[:12])
392+
stale_release.mkdir(parents=True)
393+
(stale_release / "half-written-file").write_text("junk", encoding="utf-8")
394+
stale_venv = paths.self_venv(sha2[:12])
395+
stale_venv.mkdir(parents=True)
396+
(stale_venv / "junk").write_text("junk", encoding="utf-8")
397+
398+
manager = _manager(env, src, [])
399+
status = manager.check_and_stage()
400+
401+
assert status.error is None
402+
assert status.staged_sha == sha2
403+
assert not (paths.self_release(sha2[:12]) / "half-written-file").exists()

0 commit comments

Comments
 (0)