fix(gateway): admit exactly one /update per profile - #78977
briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the gateway’s /update slash command so that exactly one update process can run per profile at a time, preventing concurrent invocations from clobbering the shared .update_* IPC markers and spawning duplicate updaters. It implements atomic admission control in the live handler (gateway/slash_commands.py), adds an “already running” localized user message across all locales, and introduces concurrency-focused regression tests.
Changes:
- Add
_claim_update_slot()usingos.open(..., O_CREAT | O_EXCL)+ TTL takeover to atomically reserve the profile-wide update slot before writing routing metadata and spawning the updater. - Return a new localized “already running” response when a second
/updateis rejected, while still scheduling the notification watcher. - Add a new
TestUpdateAdmissionControlsuite covering exclusivity, TTL reclaim, and real concurrent callers without pre-seeding markers.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
gateway/slash_commands.py |
Adds atomic /update slot claiming with TTL, integrates admission control into _handle_update_command, and returns the new gateway.update.already_running message on rejection. |
tests/gateway/test_update_command.py |
Adds a concurrency-focused test class validating exclusive admission, TTL reclaim, and single-updater spawning under parallel callers. |
locales/af.yaml |
Adds gateway.update.already_running translation. |
locales/ar.yaml |
Adds gateway.update.already_running translation. |
locales/de.yaml |
Adds gateway.update.already_running translation. |
locales/en.yaml |
Adds gateway.update.already_running translation. |
locales/es.yaml |
Adds gateway.update.already_running translation. |
locales/fr.yaml |
Adds gateway.update.already_running translation. |
locales/ga.yaml |
Adds gateway.update.already_running translation. |
locales/hu.yaml |
Adds gateway.update.already_running translation. |
locales/it.yaml |
Adds gateway.update.already_running translation. |
locales/ja.yaml |
Adds gateway.update.already_running translation. |
locales/ko.yaml |
Adds gateway.update.already_running translation. |
locales/pt.yaml |
Adds gateway.update.already_running translation. |
locales/ru.yaml |
Adds gateway.update.already_running translation. |
locales/tr.yaml |
Adds gateway.update.already_running translation. |
locales/uk.yaml |
Adds gateway.update.already_running translation. |
locales/zh.yaml |
Adds gateway.update.already_running translation. |
locales/zh-hant.yaml |
Adds gateway.update.already_running translation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if claimed_path.exists() or not _claim_update_slot( | ||
| pending_path, _UPDATE_RESERVATION_TTL_S | ||
| ): | ||
| # The loser still gets the outcome: the watcher is profile-wide and | ||
| # reports the result into the chat that started the update. | ||
| self._schedule_update_notification_watch() | ||
| return t("gateway.update.already_running") | ||
|
|
There was a problem hiding this comment.
Confirmed and fixed in d15ce760e (current head). This was a real second-updater admission, not a theoretical window — reproduced before the fix, with the exact interleaving you described:
AssertionError: Expected 'Popen' to not have been called. Called 1 times.
Calls: [call(['/usr/bin/setsid', 'bash', '-c',
'PYTHONUNBUFFERED=1 /usr/bin/hermes update --gateway > .../.update_output.txt ...'])]
The fix follows your suggestion: re-check claimed_path immediately after a successful claim and release the reservation if it is present.
admitted = not claimed_path.exists() and _claim_update_slot(
pending_path, _UPDATE_RESERVATION_TTL_S
)
if admitted and claimed_path.exists():
_release_update_slot(pending_path)
admitted = FalseOne thing worth calling out on the release side. A plain pending_path.unlink() reintroduces a smaller version of the same problem in the opposite direction: if the notifier's claimed_path.replace(pending_path) lands between the re-check and the unlink, the marker is no longer this caller's empty reservation but the live update's restored routing metadata, and deleting it would silently lose that user's completion notice. So _release_update_slot only removes the marker while it is still zero-length — the state the exclusive create leaves it in, and one nothing else produces:
def _release_update_slot(pending_path: Path) -> None:
try:
if pending_path.stat().st_size == 0:
pending_path.unlink()
except OSError:
passTwo regression tests, both of which fail against the previous commit 58ac06ac6:
tests/gateway/test_update_command.py::TestUpdateAdmissionControl::test_rejects_when_notifier_claims_a_live_update_mid_admission— writes the claimed marker from inside the admission window to stand in for the concurrent notifier, then asserts no spawn, an "already running" reply, the reservation handed back, and the live update's claimed marker untouched.tests/gateway/test_update_command.py::TestUpdateAdmissionControl::test_release_update_slot_keeps_a_marker_that_holds_metadata— pins the zero-length condition, so a marker carrying metadata survives a release.
tests/gateway/ is green apart from seven failures that are identical on clean origin/main (test_discord_send, test_send_multiple_images, test_session_store_prune, test_shutdown_forensics, test_systemd_notify) and are order-dependent — they pass in isolation.
There was a problem hiding this comment.
Correction to the SHA above: d15ce760e was orphaned when this branch was rebased onto current main. The fix is live on head as 0e2af35db73 ("fix(gateway): re-check the claimed marker after reserving the update slot"), sitting on 2ddeed6dcb6 ("fix(gateway): admit exactly one /update per profile").
The mechanism is unchanged. The claimed_path.exists() pre-check is no longer load-bearing on its own — after _claim_update_slot wins the exclusive create, the claimed marker is re-checked, and if the notifier's pending -> claimed rename landed in the window you identified, the reservation is handed back and the caller takes the loser path:
admitted = not claimed_path.exists() and _claim_update_slot(
pending_path, _UPDATE_RESERVATION_TTL_S
)
if admitted and claimed_path.exists():
_release_update_slot(pending_path)
admitted = False_release_update_slot only unlinks the marker while it is still the empty file the claim created, so a marker the notifier has already restored via claimed_path.replace(pending_path) — someone else's routing metadata — is left alone rather than deleted.
Rebase-proof anchor, since the SHA will move again: the regression is test_rejects_when_notifier_claims_a_live_update_mid_admission in tests/gateway/test_update_command.py, which drives the rename precisely between the check and the create. Grep that name rather than the commit id.
alt-glitch
added
type/bug
`/update` wrote `.update_pending.json` and spawned a detached `hermes update --gateway` with no admission control of any kind. Two concurrent invocations — a user double-tapping because the update runs for minutes with no acknowledgement, or two platforms on one multiplexed gateway both triggering it — each wrote the marker and each spawned an updater against the same checkout and virtualenv. The second write also replaced the first requester's routing metadata, so that user never learned their update finished. Both handlers also stage through the same `.update_pending.tmp` path, so the losing rename raises FileNotFoundError out of the handler. Reserve the profile-wide slot with os.open(O_CREAT | O_EXCL) before any routing metadata is written and before the updater is spawned. That collapses observe-and-claim into a single atomic syscall, so exactly one caller can ever win. The losing caller gets an "update already running" reply and still gets `_schedule_update_notification_watch()`, so it learns the outcome of the update that is actually running. The reservation carries a TTL: an updater killed before it can write its exit code (host reboot, OOM kill) would otherwise wedge /update for the lifetime of the profile. Any failure between the claim and the spawn releases the slot immediately so the user can retry. Supersedes NousResearch#15539, which identified this collision. That guard patched `gateway/run.py`, where the handler no longer lives after 619bd78, and used a check-then-write `if pending_path.exists(): return` that both callers can pass; its tests pre-seeded the marker, so they never exercised the check-to-create gap.
…slot The `claimed_path.exists()` pre-check was not synchronized with the exclusive create. `_send_update_notification` renames `.update_pending.json` -> `.update_pending.claimed.json` while an update is in flight; landing that rename between the check and the create leaves `pending` momentarily absent, so `_claim_update_slot` succeeds and admits a second updater against the live one. The notifier's later `claimed.replace(pending)` then clobbers the new reservation. Re-check the claimed marker immediately after a successful claim and hand the reservation back if it is present. `_release_update_slot` only removes the marker while it is still the empty file the claim created, so a marker the notifier has already restored — someone else's routing metadata — survives instead of being deleted.
d15ce76 to
0e2af35
Compare
Supersedes #15539
@hharry11's #15539 identified this collision correctly and the underlying overwrite path is still live on
main. This PR re-does that fix against the code as it exists today, and closes the three specific deficiencies called out in review./updateinvocations clobber the shared.update_pending.jsonIPC state, and a second updater is spawned.gateway/run.py:7821, but the handler moved togateway/slash_commands.pyin619bd7827, so its guard never executes on HEAD (that PR is alsoCONFLICTING/DIRTYtoday). Its guard wasif pending_path.exists() or claimed_path.exists(): return, a check-then-write that both callers can pass. Its two tests pre-seeded the marker, so they never reached the check-to-create gap./updateforever, and a synchronized two-caller regression that pre-seeds nothing.What does this PR do?
_handle_update_commandingateway/slash_commands.pywrites.update_pending.jsonand spawns a detachedhermes update --gatewaywith no admission control of any kind. Agrep -niE "claimed|lock|already|in_progress|is_running|O_EXCL"over the whole handler body onmainreturns nothing./updateis profile-global: it rewrites the checkout and the virtualenv every session on the host shares, and the.update_*markers are a single-slot mailbox holding one requester's routing metadata. Two concurrent invocations — a user double-tapping because the update runs for minutes with no acknowledgement, or two platforms on one multiplexed gateway both triggering it — each write the marker and each spawn an updater against the same checkout and venv. The second write replaces the first requester's routing metadata, so that user never learns their update finished.There is a second, louder symptom: both handlers stage through the same
.update_pending.tmppath, so the losingPath.replace()raisesFileNotFoundErrorstraight out of the handler. That is reproduced by the new concurrency test against unpatchedmain— see How to Test.The fix reserves the profile-wide slot with
os.open(..., O_CREAT | O_EXCL)before any routing metadata is written and before the updater is spawned, collapsing observe-and-claim into a single atomic syscall so exactly one caller can ever win.O_EXCLis the same primitive on Windows (CPython raisesFileExistsErrorthere too), so the win32 spawn branch needs no separate handling.Related Issue
No separate issue — this supersedes PR #15539, whose review states the acceptance criteria. Deliberately not using a closing keyword, so #15539 stays open for its author to close.
Acceptance checklist from the #15539 review
Answered by
_claim_update_slot, which usesos.open(O_CREAT | O_EXCL). There is no window between observing the slot is free and taking it — it is one syscall, and the kernel picks the winner.test_claim_update_slot_is_exclusive_under_parallel_callersreleases 16 threads from a barrier onto one path and asserts exactly oneTrue.The guard is in
gateway/slash_commands.py, in_handle_update_command. The cited lines have drifted since the review — the handler now starts at:5393and the unguarded write is at:5450-5453on36cb5ae55. Nothing is changed ingateway/run.py.No test in this PR pre-seeds a marker to stand in for the race. Every marker the guard is handed is created by the handler or by the primitive under test.
test_concurrent_update_commands_spawn_exactly_one_updaterruns two callers in real threads against the real filesystem, released together from a barrier placed at the last step before the claim, and asserts exactly one spawn plus one "already running" reply.test_second_update_is_rejected_without_preseeding_a_markerdrives the reject path purely from state the first call produced, and asserts the winner's routing metadata survived byte-for-byte.The one test that does write a marker,
test_rejects_when_notifier_claims_a_live_update_mid_admission, writes it from inside the admission window to stand in for the concurrent notifier — that is the event being simulated, not state handed to the guard up front. See Follow-up below.Sibling-site sweep
git grep -n "update_pending" origin/main -- '*.py'(non-test), every site classified:gateway/slash_commands.py:5450-5453gateway/run.py:11281-11282gateway/run.py:20564-20565_watch_update_progressreads claimed→pending for routinggateway/run.py:20806-20824_send_update_notification— already claims atomically viapending_path.replace(claimed_path)The three
run.pysites only ever consume the marker; none of them creates one, so none of them can produce a second updater. The reservation deliberately mirrors the claim idiom the reader atgateway/run.py:20819already uses, so the two halves of the protocol match.tests/gateway/test_update_streaming.pyexercises the watcher (a reader) and needed no change; it still passes unmodified.Changes Made
gateway/slash_commands.py_claim_update_slot(pending_path, ttl_seconds)— atomicO_CREAT | O_EXCLreservation, with takeover of a reservation older than the TTL._UPDATE_RESERVATION_TTL_S = 3600.0. An updater killed before it writes its exit code (host reboot, OOM kill) leaves a marker nothing will clean up; without a ceiling the new guard would wedge/updatefor the lifetime of the profile — a worse bug than the one being fixed. Sized above_watch_update_progress's own 1800s watch timeout so a live, still-watched update is never stolen._handle_update_commandclaims the slot before writing routing metadata and before spawning. The loser getst("gateway.update.already_running")and_schedule_update_notification_watch(), so it still learns how the running update turned out.try, and the failure path releases the reservation (and any orphaned.update_pending.tmp), so a spawn that never started does not block the next/update.locales/*.yaml(all 17) — newgateway.update.already_runningkey.tests/agent/test_i18n.pyasserts catalog parity, so the key has to land in every locale in the same commit.tests/gateway/test_update_command.py— newTestUpdateAdmissionControlwith 6 tests.How to Test
gateway/slash_commands.pytoorigin/mainand re-run the new class:main:test_failed_spawn_releases_the_update_slotis the one that passes both ways by design — it guards the new release path, i.e. that this PR cannot itself wedge/update, so it has nothing to fail against onmain.)tests/gateway/suite, run serially on this branch and on cleanorigin/mainfor comparison: the identical 7 failures on both, intest_discord_send.py,test_send_multiple_images.py,test_session_store_prune.py,test_shutdown_forensics.py,test_systemd_notify.py. All are pre-existing and order-dependent (they pass in isolation), none is in touched code, and the set does not change with this PR applied./updatetwice in quick succession. Before, twohermes updateprocesses appear (pgrep -fa "hermes update") and only the second chat is notified. After, the second call answers "A Hermes update is already running for this profile" and one updater runs.Follow-up: second-updater window closed in
d15ce760eCopilot's review caught a real defect in the first commit, and it is worth recording because it is the same class of bug as the one this PR fixes. The
claimed_path.exists()pre-check was not synchronized with the exclusive create._send_update_notificationrenames.update_pending.json→.update_pending.claimed.jsonwhile an update is in flight; landing that rename between the check and the create leavespendingmomentarily absent, so the claim succeeded and admitted a second updater against the live one — and the notifier's laterclaimed.replace(pending)clobbered the new reservation. Reproduced, not theoretical:d15ce760ere-checks the claimed marker immediately after a successful claim and hands the reservation back if it is present. The release itself is narrowed:_release_update_slotonly removes the marker while it is still zero-length — the state the exclusive create leaves it in — because if the notifier restored a live update's metadata in between, deleting it would lose that user's completion notice.Both new tests (
test_rejects_when_notifier_claims_a_live_update_mid_admission,test_release_update_slot_keeps_a_marker_that_holds_metadata) fail against58ac06ac6and pass ond15ce760e. 8 tests inTestUpdateAdmissionControl, 20 in the file.Related / Positioning
Both dedup nets were run; stating what was checked rather than claiming the field is empty.
By changed file (
gh pr list --json files, newest 300 open PRs, spanning fix(gateway): run /insights, /debug and /goal draft inside the routed profile #78440–Fix gateway tips for surface-specific commands #78871 — roughly the last day, so this net sees recent rivals only): 6 open PRs touchgateway/slash_commands.py, none on the update path.By symbol, corpus-wide (
gh search prson_handle_update_command,update_pending,slash_commands.py,15539): the two adjacent PRs worth naming are.update_in_progresssentinel, but ingateway/run.pywhere the handler no longer lives, and the sentinel is read with the same check-then-write pattern the fix: prevent concurrent gateway updates from clobbering shared IPC state #15539 review rejected. It is bundled with five unrelated concerns (transcript logging, UTF-8 env, Windows helper streaming) across 6 files. Overlapping intent, but it does not make concurrent admission safe and does not execute on HEAD./updateand extracts the spawn into_execute_update. Different concern — a UX gate, not admission control; two callers who both confirm still both spawn. If it lands first this PR rebases onto_execute_updatecleanly, since the claim belongs at the top of the spawn path either way.Neither is a superset of this change on the atomicity dimension. Happy to defer or rebase if a maintainer reads it differently.
Type of Change
Checklist
Code
fix(scope):,feat(scope):, etc.)tests/gateway/package (which contains every test this PR touches) plustests/agent/test_i18n.pyfor catalog parity, run serially against both this branch and cleanorigin/main— identical results, zero new failures. Details in How to Test item 3.Documentation & Housekeeping
docs/, docstrings) — the new helper and the TTL constant carry docstrings/comments explaining the invariant; no user docs describe/updateconcurrencyO_CREAT | O_EXCLraisesFileExistsErroron Windows as on POSIX, so the win32 spawn branch needs no separate guard; only the metadata write moved inside the existingtry, which is platform-agnostic. Tested on macOS.