sysadminandClaude Opus 5
220361ad94
Addresses the two blockers raised in the PR #886 review (comment 16559) for
issue #663.
B1 — apply_authorized ignored restart-class authorization.
The #663 restart-class matrix and the #661 drain-proof hard gate are
independent authorizations that first coexisted when PR #882 landed on
master and this branch merged it. The union preserved both, but the apply
decision consulted only the drain gate:
payload["apply_authorized"] = gate.allow
so a clean drain proof — or an authorized break-glass, which needs no proof
at all — reported apply_authorized: True for a class the least-privilege
matrix had just denied, in the same payload carrying allow_restart: False
and "role 'author' may not request full_mcp_restart". One environment
variable therefore collapsed the whole nine-class matrix for the apply
decision, including host_restart.
The apply decision is now the conjunction of both authorizations, and
apply_gate carries drain_gate_allow and restart_class_authorized so a denial
is attributable to the authorization that produced it. Break-glass keeps its
purpose — bypassing the drain proof — and never bypasses the class matrix.
No existing fail-closed behaviour is weakened: allow_restart, drain-proof
verification, fingerprint binding, and requester authorization are untouched.
B2 — docs/mcp-restart-coordinator.md described pre-#661 behaviour.
The document still called the drain proof "a separate child" and omitted
drain_proof_json and request_break_glass from the published signature, so a
safety document asserted there was no gate where a gate now exists. It now
documents both parameters, states that the gate executes inside this tool,
and records dry-run versus apply behaviour, authorization ordering, the
break-glass scope, and fail-closed conditions as implemented.
Regression coverage.
tests/test_issue_886_apply_authorization_conjunction.py exercises the MCP
tool itself, which previously had no test at all — that absence is why the
defect shipped. It pins both conjunction directions, proves a clean proof
cannot override a role, approval, unknown-class, or missing-target denial,
proves break-glass does not collapse the matrix for any worker role or
restricted class, and proves the existing scoped and unscoped paths and the
#661 denials still hold. Against the pre-fix tree 24 of these fail; against
this commit all 19 pass with 45 subtests.
tests/test_mcp_restart_governance_docs.py now binds the published signature
to inspect.signature() of the real tool and forbids the stale pre-#661
phrasing, so the drift that produced B2 cannot return unnoticed.
Verification: targeted restart/drain/governance/webui suites 194 passed,
113 subtests. Full suite 23 failed, 5230 passed, 6 skipped, 912 subtests —
the failure set is identical to the reviewed baseline at 9bc021e
(23 failed, 5201 passed), with +29 passing from the added tests and no new
or changed failure. Zero conflict markers; py_compile passes; the #882
union remains intact in both directions.
Refs #663, PR #886
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01V6xFqovhbArPv61j9KCGkL
153 lines
7.7 KiB
Markdown
153 lines
7.7 KiB
Markdown
# MCP restart coordinator and impact analysis (#658)
|
|
|
|
Before any sanctioned MCP restart, a central coordinator evaluates the live
|
|
control-plane state and produces an **impact preview** so operators and the web
|
|
console (#642 / #652) can see the blast radius *before* concurrent LLM work is
|
|
disrupted. Uncoordinated restarts destroy in-flight author/reviewer/merger work
|
|
and give operators no way to see what they are about to break.
|
|
|
|
This lands the coordinator + impact DTO + the MCP tool. It is the single
|
|
sanctioned entry point for restart evaluation post-#657 (which inventoried the
|
|
restart/reload/kill paths).
|
|
|
|
The **drain-proof hard gate now executes inside this tool** (#661, via PR #882):
|
|
an apply request (`dry_run=False`) is evaluated against a drain proof here and
|
|
denied when that proof is missing, expired, unclean, tampered with, or stale.
|
|
It is no longer a separate child operation. What remains a later child is only
|
|
the **execution** step — actually stopping and restoring a process. This tool
|
|
still never restarts anything: `apply_supported` is always `false` and
|
|
`restart_performed` is always `false`.
|
|
|
|
The coordinator now routes every request through the restart-class policy
|
|
matrix defined for #663. See
|
|
[`mcp-restart-classes.md`](./mcp-restart-classes.md) for permissions, expected
|
|
blast radius, scoped drain and approval requirements, audit fields, and
|
|
recovery behavior for all nine classes.
|
|
|
|
## Components
|
|
|
|
| Piece | Where | Responsibility |
|
|
|-------|-------|----------------|
|
|
| `restart_coordinator.evaluate_restart_impact` | `restart_coordinator.py` | Pure classification: inventory → impact report DTO. No I/O, no restart. |
|
|
| `RestartImpactReport` / `SessionImpact` / `LeaseImpact` | `restart_coordinator.py` | Console-facing DTO (`.as_dict()` is JSON-serializable). |
|
|
| `ControlPlaneDB.list_sessions` | `control_plane_db.py` | Read-only session inventory (the process-level unit a restart kills). |
|
|
| `gitea_request_mcp_restart` | `gitea_mcp_server.py` | MCP tool: gathers inventory from the #613 DB, calls the coordinator, returns the report, and on `dry_run=False` runs the #661 drain-proof hard gate. Never restarts a process. |
|
|
| `drain_proof.gate_apply_restart` | `drain_proof.py` | The #661 hard gate: verifies a drain proof against the current impact fingerprint, or records an authorized break-glass bypass. |
|
|
|
|
## Dimensions evaluated
|
|
|
|
The coordinator classifies the inventory across the dimensions #658 requires:
|
|
|
|
- **Sessions** — every active MCP session; a restart terminates all of them.
|
|
Liveness = `status == active` **and** the owner pid is alive **and** the
|
|
heartbeat is fresh (default window 15 min). Dead/stale sessions do not count
|
|
toward blast radius.
|
|
- **Leases / locks** — control-plane leases joined with work items and their
|
|
freshness (`lease_lifecycle.classify_lease_freshness`). Only `active` (live
|
|
owner) leases are *disruptive*; expired / released / dead-process leases never
|
|
withhold a restart.
|
|
- **Issue / PR work** — the issues and PRs behind disruptive leases.
|
|
- **Mutations / critical sections** — a live lease carrying an author worktree
|
|
or a mutating phase (`implementing`, `publishing`, `merging`, …) is a
|
|
critical section a restart must not sever.
|
|
- **Terminal (merge) lock** — an active terminal lock always makes a restart
|
|
unsafe.
|
|
- **Prior recovery attempts** — narrower recovery already tried (e.g. sanctioned
|
|
client reconnects) is echoed so the operator sees the escalation history.
|
|
|
|
## Verdict
|
|
|
|
Exactly three verdicts, matching the acceptance criteria:
|
|
|
|
| Verdict | `allow_restart` | Meaning |
|
|
|---------|-----------------|---------|
|
|
| `safe` | `true` | No other live sessions, no live leases, no terminal lock. |
|
|
| `unsafe` | `false` | Live work would be disrupted and no operator override is present — **or** the inventory could not be completed (fail closed). |
|
|
| `override` | `true` | Live work present, but an operator override accepts the blast radius. |
|
|
|
|
`override_would_allow` tells the console whether an override path exists for the
|
|
current state. `blast_radius` is a `none` / `low` / `medium` / `high` severity
|
|
band derived from the affected session and work counts.
|
|
|
|
### Fail closed
|
|
|
|
If the control-plane inventory cannot be completed (DB unavailable, a listing
|
|
failed), `inventory_complete` is `false` and the verdict is `unsafe` / deny. An
|
|
incomplete evaluation must never green-light a restart.
|
|
|
|
### Operator override authority
|
|
|
|
Override authority is read from the environment variable
|
|
`GITEA_OPERATOR_RESTART_OVERRIDE_AUTHORIZATION` and **never** from a tool
|
|
argument. A worker session cannot set an environment variable on an
|
|
already-running daemon, so override cannot be self-asserted (same pattern as the
|
|
#630 daemon-maintenance authorization). The `request_override` tool argument only
|
|
expresses caller intent; it takes effect solely when the environment
|
|
authorization is present.
|
|
|
|
## The tool
|
|
|
|
```text
|
|
gitea_request_mcp_restart(remote, host, org, repo,
|
|
dry_run=True, request_override=False,
|
|
session_id=None, limit=200,
|
|
restart_class="full_mcp_restart",
|
|
target_session_id=None, target_role=None,
|
|
target_connector=None,
|
|
drain_proof_json=None,
|
|
request_break_glass=False)
|
|
```
|
|
|
|
It **never restarts anything**: `apply_supported` is always `false` and
|
|
`restart_performed` is always `false`.
|
|
|
|
### Dry-run versus apply
|
|
|
|
| Call | Behavior |
|
|
|------|----------|
|
|
| `dry_run=True` (default) | Read-only impact preview. No drain proof is required or consulted. |
|
|
| `dry_run=False` | The #661 drain-proof hard gate runs **in this tool**. The outcome is reported under `apply_gate` / `apply_authorized`; a denial also returns a durable `incident` descriptor. Still no restart. |
|
|
|
|
### Authorization ordering
|
|
|
|
An apply requires **both** authorizations, and they are independent:
|
|
|
|
1. **Restart-class authorization** (#663) — the requester's role and permissions
|
|
must allow the requested class, the class's approval requirement must be
|
|
satisfied, and any target-scoped class must name its target. Failing any of
|
|
these makes `allow_restart` `false`.
|
|
2. **Drain-proof gate** (#661) — a valid, unexpired, clean proof bound to the
|
|
current impact fingerprint, or an authorized break-glass.
|
|
|
|
`apply_authorized` is the conjunction: `gate.allow and allow_restart`. A clean
|
|
drain proof therefore cannot override a class or requester-role denial, and a
|
|
denied class never reports an authorized apply. `apply_gate` carries
|
|
`drain_gate_allow` and `restart_class_authorized` so a denial is attributable to
|
|
the authorization that produced it.
|
|
|
|
### Break-glass
|
|
|
|
Break-glass bypasses the **drain proof only** — never the restart-class matrix.
|
|
It is honoured solely when `request_break_glass` is set *and* the environment
|
|
carries `GITEA_BREAKGLASS_RESTART_AUTHORIZATION`; like operator override, the
|
|
tool argument expresses caller intent and cannot be self-asserted by a worker
|
|
session. `break_glass_requested` and `break_glass_authorized` are both reported,
|
|
so a bypass is never silent.
|
|
|
|
### Fail closed on apply
|
|
|
|
A missing, malformed, expired, unclean, tampered, or fingerprint-stale drain
|
|
proof denies the apply and returns an `incident` descriptor. An unknown restart
|
|
class denies before any of this. Ambiguity always denies.
|
|
|
|
## Audit
|
|
|
|
Every evaluation carries an `audit_record` (event, coordinator version, verdict,
|
|
restart class, required permission, allow decision, blast radius, counts,
|
|
timestamp) so restart decisions are
|
|
auditable. No secrets flow through the coordinator — session ids, pids, and
|
|
profiles are operational metadata only.
|
|
|
|
A representative dry-run report is in
|
|
[`mcp-restart-impact-sample.json`](./mcp-restart-impact-sample.json).
|