Scaled-Tech-Consulting/Gitea-Tools
2
0
Fork 0
Code Issues 102 Pull Requests 11 Actions Packages Projects Releases Wiki Activity

fix(runtime): recognize client identity environment and refresh worker registrations

Browse Source 
Two defects left behind by #948 made sanctioned multi-client operation
impossible. They are inseparable: fixing either alone still leaves the
multi-client canary unable to run.

1. Client-identity environment keys were not recognized.

   gitea_mcp_server reads GITEA_MCP_CLIENT, GITEA_MCP_CLIENT_INSTANCE and
   GITEA_MCP_CLIENT_SESSION as the authoritative client-identity inputs for
   worker registration, but none of the three appeared in
   RECOGNIZED_GITEA_ENV_KEYS or matched a recognized prefix. The runtime
   diagnostic scans every peer mcp_server.py process environment and classifies
   any unlisted GITEA_* key as an unsupported override, which is raised as a
   runtime blocker, so gitea_resolve_task_capability returned
   blocker_kind=runtime_reconnect_required with stop_required=true. Because that
   resolver is the mandatory preflight for every author, reviewer and merger
   mutation, setting the very variable #948 requires closed the mutation gate
   for the whole fleet, and reconnecting could not clear it: the variable is
   re-exported from the client's server definition on every launch.

   The three keys are now named individually in the recognized-key set. No
   prefix is added, so an unrecognized GITEA_* override is still refused
   exactly as before.

   A related inconsistency in the same path is also fixed. The diagnostic
   reasons are raised as one RuntimeError, but the preflight re-raise
   recognized only "stale-runtime:", so an "unsupported-env:" reason was
   silently swallowed there while still failing the resolver. Both reason
   families now live in RUNTIME_DIAGNOSTIC_HARD_PREFIXES beside the function
   that produces them, and both propagate identically. This only widens what is
   refused, never what is permitted.

2. WorkerRegistry.heartbeat() had no production caller.

   #948 delivered heartbeat() but only tests called it. The single production
   writer registers once per process behind an attempted-once flag, and
   register() stamps the same timestamp into both started_at and
   last_heartbeat_at. Nothing advanced it afterwards: no lifespan hook, no
   background task, no atexit handler in a process that blocks in mcp.run().
   Since liveness is age against heartbeat_ttl_seconds, that TTL was not a
   liveness window at all but a hard cap on how long any client could stay
   attached; at 900 seconds a healthy, connected, client-managed process became
   session_ownership=unowned with blocker_kind=session_attachment_missing.

   WorkerHeartbeatSupervisor in mcp_worker_identity is the missing caller,
   started from _active_worker_identity() at the moment register() succeeds,
   because that is the only point where identity and fencing_epoch are both
   known. It is a daemon thread rather than an asyncio task or a request-driven
   refresh because renewal must survive an idle session, and because the
   registry performs blocking BEGIN IMMEDIATE sqlite writes that must not run on
   the server's event loop. daemon=True is deliberate: a hard kill takes the
   thread with it, so a dead worker still goes stale on the normal TTL.

   heartbeat_interval_for() returns one third of the TTL, hard-capped at one
   half, so two consecutive beats can be lost without the row expiring and no
   override can produce an interval that outlives the registration it renews.
   heartbeat() gains optional keyword-only expectations (session, generation,
   client name, pid); each supplied one must match the recorded row or the
   renewal is refused with the existing BLOCKER_FENCED literal rather than a new
   blocker_kind, since consumers switch on that value. Omitting them preserves
   the pre-existing behavior exactly. Client names are compared normalized, so
   several namespaces of one application stay one client while separate
   applications stay distinct.

   A terminal refusal stops the supervisor permanently and records why, so a
   fenced session can never beat its way back into ownership. A transient
   failure is counted and beating continues. An atexit hook stops it on orderly
   shutdown. status() is surfaced read-only as worker_heartbeat on
   gitea_get_runtime_context so a stopped heartbeat is diagnosable before the
   TTL turns it into session_attachment_missing; it grants nothing.

   claim_generation() still has no production caller. It bumps fencing_epoch,
   which would fence the supervisor's cached epoch, and the strict refusal is
   left in place deliberately: auto-re-adopting a bumped epoch would defeat
   fencing.

No lock or lease TTL is changed, including the author issue-lock TTL, and no
mutation refused today becomes permitted.

Tests: tests/test_issue_975_client_identity_heartbeat.py adds 40 focused tests
covering all 13 acceptance criteria. Every TTL assertion uses an injected
clock; no test waits for a real TTL. The thread-loop tests use a
millisecond-scale interval with bounded polling.

Focused: 40 passed, 15 subtests passed.
Full suite from inside the branches worktree: 28 failed, 6105 passed, 6 skipped,
1105 subtests — an identical failure set to the 324a0c8a baseline measured in a
sibling branches worktree (28 failed, 6065 passed, 1090 subtests). Zero new
failures; the delta is exactly the added tests.

Closes #975

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
jcwalker3
2026-07-29 20:06:10 -05:00
co-authored by Claude Opus 4.8
parent 324a0c8a8d
commit 1a38ef95e3
4 changed files with 1268 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
mcp_worker_identity.py
+386
View File
@@ -29,6 +29,7 @@ implements:
from __future__ import annotations
import atexit
import hashlib
import os
import re
@@ -123,6 +124,103 @@ DEFAULT_REGISTRY_PATH = os.path.expanduser(
#: worker within a single operator coffee break.
DEFAULT_HEARTBEAT_TTL_SECONDS = 900.0
#: Optional operator override for the beat interval, in seconds. Clamped by
#: :func:`heartbeat_interval_for` so it can never be set at or above the TTL.
HEARTBEAT_INTERVAL_ENV = "GITEA_WORKER_HEARTBEAT_INTERVAL_SECONDS"
#: Never beat faster than this, so a misconfigured interval cannot turn the
#: supervisor into a busy sqlite writer.
MIN_HEARTBEAT_INTERVAL_SECONDS = 1.0
def heartbeat_interval_for(
ttl_seconds: float = DEFAULT_HEARTBEAT_TTL_SECONDS,
override: str | float | None = None,
) -> float:
"""Beat interval for *ttl_seconds*: one third of the TTL, capped at a half.
#975. A third means two consecutive beats can be lost before the row is
allowed to look stale, and the half-TTL cap is a hard ceiling so no
override can produce an interval that expires the registration it is
supposed to renew. That is the whole liveness contract: a healthy worker
stays owned, and a worker that has genuinely stopped beating still expires
on the configured TTL — this function never touches the TTL itself.
"""
try:
ttl = float(ttl_seconds)
except (TypeError, ValueError):
ttl = DEFAULT_HEARTBEAT_TTL_SECONDS
if not ttl > 0:
ttl = DEFAULT_HEARTBEAT_TTL_SECONDS
interval = ttl / 3.0
if override is not None:
candidate = str(override).strip()
if candidate:
try:
parsed = float(candidate)
except (TypeError, ValueError):
parsed = None
if parsed is not None and parsed > 0:
interval = parsed
ceiling = ttl / 2.0
if interval > ceiling:
interval = ceiling
if interval < MIN_HEARTBEAT_INTERVAL_SECONDS:
interval = min(MIN_HEARTBEAT_INTERVAL_SECONDS, ceiling)
return interval
#: Recorded-vs-presented pairs compared by :func:`_heartbeat_expectation_drift`.
_HEARTBEAT_EXPECTATION_FIELDS = (
"session_id",
"generation_id",
"client_name",
"pid",
)
def _heartbeat_expectation_drift(
record: dict[str, Any],
*,
expected_session_id: str | None = None,
expected_generation_id: str | None = None,
expected_client_name: str | None = None,
expected_pid: int | None = None,
) -> list[tuple[str, Any, Any]]:
"""Return ``(field, recorded, presented)`` for every mismatched expectation.
Only supplied expectations are compared, so a caller that presents nothing
gets the pre-#975 behaviour. ``client_name`` is compared through
:func:`normalize_client_name` so one application's namespaces stay one
client while genuinely different clients stay distinct.
"""
presented = {
"session_id": expected_session_id,
"generation_id": expected_generation_id,
"client_name": (
normalize_client_name(expected_client_name)
if expected_client_name is not None
else None
),
"pid": expected_pid,
}
drift: list[tuple[str, Any, Any]] = []
for field in _HEARTBEAT_EXPECTATION_FIELDS:
want = presented[field]
if want is None:
continue
got = record.get(field)
if field == "pid":
match = got is not None and int(got) == int(want)
else:
match = got == want
if not match:
drift.append((field, got, want))
return drift
STATUS_ACTIVE = "active"
STATUS_SUPERSEDED = "superseded"
STATUS_RELEASED = "released"
@@ -786,11 +884,25 @@ class WorkerRegistry:
worker_identity: str,
fencing_epoch: int,
now: datetime | None = None,
expected_session_id: str | None = None,
expected_generation_id: str | None = None,
expected_client_name: str | None = None,
expected_pid: int | None = None,
) -> dict[str, Any]:
"""Renew only the owning registration (#948 AC11).
A stale epoch is refused rather than silently renewed, so a superseded
session that resumes cannot heartbeat its way back into ownership.
#975 adds optional keyword-only *expectations*. Every one supplied must
match the recorded row or the renewal is refused. They exist because
identity plus epoch cannot express "renew the row I registered, and only
that row": a recycled PID, or a second session of the same client, would
otherwise be renewable by the wrong beater. They are fencing tokens,
never assertions — supplying one can only cause a refusal, never grant
anything, and omitting them preserves the pre-#975 behaviour exactly.
Drift reports the existing ``BLOCKER_FENCED`` rather than a new
``blocker_kind``, because consumers switch on that value.
"""
stamp = _ts(now or _utc_now())
with self._tx() as conn:
@@ -807,6 +919,30 @@ class WorkerRegistry:
"reasons": [f"no registration for {worker_identity!r}"],
}
record = self._row_to_record(row)
drift = _heartbeat_expectation_drift(
record,
expected_session_id=expected_session_id,
expected_generation_id=expected_generation_id,
expected_client_name=expected_client_name,
expected_pid=expected_pid,
)
if drift:
return {
"success": False,
"renewed": False,
"mutation_performed": False,
"blocker_kind": BLOCKER_FENCED,
"expectation_drift": drift,
"reasons": [
"presented worker expectations do not match the recorded "
"registration, so this beater does not own the row: "
+ "; ".join(
f"{field} recorded {recorded!r}, presented {presented!r}"
for field, recorded, presented in drift
)
+ " (#975)"
],
}
if record["status"] != STATUS_ACTIVE:
return {
"success": False,
@@ -1430,3 +1566,253 @@ def resolve_bound_remote(
"explicitly to avoid host drift (#948)."
],
}
# --- Production heartbeat lifecycle (#975) --------------------------------
#
# #948 delivered ``WorkerRegistry.heartbeat()`` and nothing ever called it, so
# ``last_heartbeat_at`` stayed pinned to ``started_at`` for every production
# worker and ``heartbeat_ttl_seconds`` stopped being a liveness window at all —
# it became a hard cap on how long any client could stay attached. This
# supervisor is the missing caller.
#
# It is a daemon thread rather than an asyncio task or a per-request refresh
# because the renewal has to survive an *idle* session: a client waiting on a
# lease with no tool call in flight must not lose ownership, which rules out
# request-driven refresh. Registration itself happens lazily inside a tool call
# on the server's event loop, and the registry performs blocking
# ``BEGIN IMMEDIATE`` sqlite writes, which must not run on that loop.
# ``daemon=True`` is deliberate: a hard kill takes the thread down with the
# process, so a dead worker still goes stale on the normal TTL.
#: Refusals that mean this worker no longer owns its row. Beating again could
#: only ever be an attempt to renew ownership it has already lost, so the
#: supervisor stops permanently instead of retrying.
TERMINAL_HEARTBEAT_BLOCKERS = frozenset({BLOCKER_FENCED, BLOCKER_NO_ATTACHMENT})
class WorkerHeartbeatSupervisor:
"""Periodically renew exactly one worker registration.
Contract:
* It never registers. A supervisor exists only for an already-registered
identity, so it cannot create a second registration or a second identity
system.
* Every beat presents the full expectation set, so it can renew only the row
matching this exact client name, worker identity, session, generation and
pid.
* A terminal refusal (fenced or missing) stops it permanently and records
why. A fenced session must never beat its way back into ownership.
* A transient failure (a locked database, say) is counted and the loop
continues, so one contended write does not silently end the heartbeat.
* Nothing here raises into a caller. ``beat_once`` returns its outcome and
the thread body swallows everything, because a heartbeat failure must
degrade to "not renewed" and never crash a tool call.
"""
def __init__(
self,
registry: WorkerRegistry,
*,
worker_identity: str,
fencing_epoch: int,
session_id: str | None = None,
generation_id: str | None = None,
client_name: str | None = None,
pid: int | None = None,
ttl_seconds: float = DEFAULT_HEARTBEAT_TTL_SECONDS,
interval_seconds: float | None = None,
clock=None,
) -> None:
self._registry = registry
self.worker_identity = worker_identity
self.fencing_epoch = int(fencing_epoch)
self.session_id = session_id
self.generation_id = generation_id
self.client_name = (
normalize_client_name(client_name) if client_name is not None else None
)
self.pid = int(pid) if pid is not None else None
self.ttl_seconds = float(ttl_seconds)
self.interval_seconds = (
heartbeat_interval_for(self.ttl_seconds)
if interval_seconds is None
else heartbeat_interval_for(self.ttl_seconds, interval_seconds)
)
#: Injectable so every TTL test uses controlled time and no test waits
#: for a real interval or a real TTL to elapse.
self._clock = clock or _utc_now
self._stop_event = threading.Event()
self._thread: threading.Thread | None = None
self._lock = threading.Lock()
self._atexit_registered = False
self.started = False
self.stopped_reason: str | None = None
self.beats_attempted = 0
self.beats_renewed = 0
self.transient_failures = 0
self.last_beat_at: str | None = None
self.last_result: dict[str, Any] | None = None
# -- one beat --
def beat_once(self, now: datetime | None = None) -> dict[str, Any]:
"""Renew once. Never raises; returns the registry outcome or a failure."""
if self.stopped_reason is not None:
return {
"success": False,
"renewed": False,
"beat_attempted": False,
"reasons": [
f"supervisor already stopped: {self.stopped_reason}"
],
}
stamp = now or self._clock()
self.beats_attempted += 1
try:
result = self._registry.heartbeat(
worker_identity=self.worker_identity,
fencing_epoch=self.fencing_epoch,
now=stamp,
expected_session_id=self.session_id,
expected_generation_id=self.generation_id,
expected_client_name=self.client_name,
expected_pid=self.pid,
)
except Exception as exc:
# Transient by assumption: an unexpected error is not proof this
# worker lost ownership, so it must not silently end the heartbeat.
self.transient_failures += 1
result = {
"success": False,
"renewed": False,
"transient": True,
"blocker_kind": None,
"reasons": [f"heartbeat raised {type(exc).__name__}: {exc}"],
}
self.last_result = result
return result
self.last_result = result
if result.get("renewed"):
self.beats_renewed += 1
self.last_beat_at = result.get("last_heartbeat_at") or _ts(stamp)
return result
blocker = result.get("blocker_kind")
if blocker in TERMINAL_HEARTBEAT_BLOCKERS:
self._stop_internal(
reason=(
f"refused with blocker_kind={blocker!r}: "
+ "; ".join(result.get("reasons") or [])
)
)
else:
self.transient_failures += 1
return result
# -- lifecycle --
def start(self) -> dict[str, Any]:
"""Start the beat thread. Idempotent; safe to call from a tool call."""
with self._lock:
if self.stopped_reason is not None:
return {
"started": False,
"reasons": [f"supervisor stopped: {self.stopped_reason}"],
}
if self._thread is not None and self._thread.is_alive():
return {"started": True, "already_running": True, "reasons": []}
self._stop_event.clear()
thread = threading.Thread(
target=self._run,
name=f"gitea-worker-heartbeat-{self.worker_identity}",
daemon=True,
)
self._thread = thread
self.started = True
if not self._atexit_registered:
# Orderly shutdown stops the heartbeat. A hard kill does not
# run this, which is correct: the row must then go stale.
atexit.register(self._atexit_stop)
self._atexit_registered = True
thread.start()
return {"started": True, "already_running": False, "reasons": []}
def _run(self) -> None:
while not self._stop_event.is_set():
# Wait first: registration already stamped a fresh heartbeat, so an
# immediate beat would be a redundant write on every server launch.
if self._stop_event.wait(self.interval_seconds):
return
try:
self.beat_once()
except Exception:
# beat_once is already total; this is the last-resort guard that
# keeps a supervisor thread from dying silently.
self.transient_failures += 1
if self.stopped_reason is not None:
return
def stop(self, reason: str = "stopped") -> dict[str, Any]:
"""Stop beating. Idempotent, and prompt because the loop waits on an Event."""
self._stop_internal(reason=reason)
thread = self._thread
if thread is not None and thread.is_alive():
thread.join(timeout=max(1.0, min(5.0, self.interval_seconds)))
return {
"stopped": True,
"reason": self.stopped_reason,
"thread_alive": bool(thread is not None and thread.is_alive()),
}
def _stop_internal(self, *, reason: str) -> None:
if self.stopped_reason is None:
self.stopped_reason = reason
self._stop_event.set()
if self._atexit_registered:
try:
atexit.unregister(self._atexit_stop)
except Exception:
pass
self._atexit_registered = False
def _atexit_stop(self) -> None:
try:
self.stop(reason="process exit")
except Exception:
pass
# -- observability --
def status(self) -> dict[str, Any]:
"""Read-only observability payload; safe to embed in a tool result."""
thread = self._thread
return {
"supervised": True,
"worker_identity": self.worker_identity,
"fencing_epoch": self.fencing_epoch,
"session_id": self.session_id,
"generation_id": self.generation_id,
"client_name": self.client_name,
"pid": self.pid,
"heartbeat_ttl_seconds": self.ttl_seconds,
"heartbeat_interval_seconds": self.interval_seconds,
"started": self.started,
"running": bool(
thread is not None
and thread.is_alive()
and self.stopped_reason is None
),
"stopped_reason": self.stopped_reason,
"beats_attempted": self.beats_attempted,
"beats_renewed": self.beats_renewed,
"transient_failures": self.transient_failures,
"last_heartbeat_at": self.last_beat_at,
"last_blocker_kind": (self.last_result or {}).get("blocker_kind"),
}