fix(fleet): dedicated retirement capability, identity proof, external fencing
Addresses the three blocking findings in review 657 on PR #982 and nothing
else.
B1 — apply was authorized by gitea.read
---------------------------------------
Plan and apply shared the observational permission class, so any profile that
could look could also destroy; the mutation landing in the local control-plane
registry rather than in Gitea makes it no less a mutation.
Apply now requires its own capability, gitea.worker_registry.retire, checked at
entry and re-resolved immediately before the registry mutation. Plan stays on
gitea.read. No profile holds the new permission by default, so author,
reviewer, merger, and read-only profiles fail closed on the permission itself
rather than on the role check alone; the controller/reconciler role restriction
remains as defence in depth. Granting it is a deliberate operator edit to
profiles.json, and removing it revokes apply completely. No new Gitea write
permission is introduced and no author permission is broadened.
B2 — complete legacy rows could be retired without identity proof
-----------------------------------------------------------------
"Incomplete identity" previously meant only that pre-existing columns were
null, which a legacy-pid row satisfies trivially. Retirement now requires an
affirmative two-part proof: launcher-minted inst- attribution, plus fencing
evidence (host_id, boot_id, process_start_time) that turns a bare pid into a
statement about one process incarnation.
New mcp_process_fencing supplies those probes; every one returns None rather
than guessing, and None always preserves. Rows written before these columns
existed, and rows on legacy instance identities, are preserved permanently —
they are retired only after re-registering under a trusted identity. That
legacy rows would otherwise remain outstanding is explicitly not treated as
grounds for a weaker proof. A live pid stays an absolute block even across a
boot boundary, and pid reuse is reported distinctly from a live worker.
B3 — lease and OS liveness sat outside the registry transaction
---------------------------------------------------------------
BEGIN IMMEDIATE locks the worker registry only, and the per-target loop runs
after revalidation, so a lease acquired or a pid revived in between would go
unnoticed — the registry-column guard cannot catch it because no registry
column changed.
Two mechanisms now close that window, both applied per target immediately
before its own write: an external_fence_fn version token over active leases,
captured inside the transaction before the authoritative read and re-compared
before every guarded UPDATE (movement aborts the whole transaction; an
unreadable lease store raises rather than comparing equal), and a liveness_fn
re-probe that must affirmatively re-establish that this exact process is gone,
comparing process_start_time so a reused pid is refused. Omitting the re-probe
retires nothing rather than proceeding unfenced. The guarded UPDATE also
asserts the fencing triple is unchanged, and all three columns participate in
the CAS token.
Preserved from the accepted work: registry_fingerprint still excludes
observation time and is order- and numeric-typing stable, plan still mutates
nothing, drift still retires zero, retired rows stay historical, and ordinary
author operations remain ungated by fleet state.
Tests: tests/test_issue_980_stale_worker_retirement.py 87 passed, 46 subtests
(was 40 passed, 23 subtests), covering all three blockers' required
regressions. Adjacent suites (#980, #978, #975, #948, task-capability role
invariants) 242 passed, 156 subtests. Full suite from the branch worktree
28 failed, 6253 passed, 6 skipped, 1152 subtests; master baseline at
108cbfa173 from branches/baseline-980-108cbfa 28 failed, 6166 passed, 6
skipped, 1106 subtests. The failing sets are identical under comm, so zero
regressions and zero masked pre-existing failures; the +87 passing delta is
this branch's tests.
No live worker-registry row was retired — every test uses a throwaway SQLite
database. BAA, issue #981, PR #906, issue #650, review 624, unrelated branches
and worktrees, profiles, configuration, credentials, sessions, and running
processes were not touched.
Refs #980
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
@@ -59,22 +59,42 @@ or contradictory evidence preserves the row.
|
||||
| `ownership_state` is exactly `stale` | `ambiguous_ownership_state` |
|
||||
| Repository binding present and canonical | `repository_binding_ambiguous` |
|
||||
| No identity evidence shared with a live or unprobeable worker | `conflicting_identity_evidence` |
|
||||
| No other active row claims the same (instance, namespace) while one may be live | `client_instance_conflict` |
|
||||
| Not an active workflow-lease owner | `protected_active_workflow_owner` |
|
||||
| Instance identity is launcher-minted (`inst-…`) | `untrusted_identity_provenance` |
|
||||
| Row's `host_id` matches the host running retirement | `host_binding_unproven` |
|
||||
| Boot identity is known on both sides | `boot_identity_unknown` |
|
||||
| The pid number is not occupied by a different incarnation | `pid_reuse_detected` |
|
||||
|
||||
Eligible rows carry `eligible_stale_orphan`.
|
||||
|
||||
Two properties are worth stating explicitly:
|
||||
Three properties are worth stating explicitly:
|
||||
|
||||
* **`pid_alive` can only withdraw liveness, never grant it**
|
||||
(`WorkerRegistry.is_live`, #948 AC7). A heartbeat-lapsed but still-running
|
||||
process therefore classifies as `stale` in the snapshot, yet #980's added
|
||||
`pid_alive is False` requirement preserves it. An unprobeable PID (`None`)
|
||||
also fails closed.
|
||||
* **Trusted launcher identity is not required.** #980 places trusted
|
||||
`client_instance_id` propagation out of scope and lists backfilling trusted
|
||||
identity for legacy workers as a non-goal. Requiring `inst-…` provenance here
|
||||
would preserve every legacy row forever and make the feature inert. What is
|
||||
required is that the registry *fields* the conjunction reads are present.
|
||||
* **Affirmative identity proof is required (review 657 B2).** An earlier
|
||||
revision required only that the pre-existing registry columns were non-null —
|
||||
which a legacy `legacy-pid-…` row satisfies trivially, so a row that proved
|
||||
nothing about *which* process it described was retireable. Retirement now
|
||||
needs both halves of a positive proof:
|
||||
* **Attribution** — a launcher-minted `inst-…` `client_instance_id`, so the
|
||||
row is known to belong to one specific application launch rather than
|
||||
having been inferred from pid proximity.
|
||||
* **Fencing** — `host_id`, `boot_id`, and `process_start_time`, which turn a
|
||||
bare pid into a statement about one process: which machine it ran on, which
|
||||
boot of that machine, and which incarnation of that pid number.
|
||||
|
||||
**Consequence, stated plainly:** registrations written before these columns
|
||||
existed, and any row on a legacy instance identity, are preserved
|
||||
*permanently*. They are retired only after their worker re-registers under a
|
||||
trusted identity — never on weaker evidence. That the alternative would leave
|
||||
legacy rows outstanding indefinitely is not a reason to relax the proof.
|
||||
* **A live pid is an absolute block.** Even across a boot boundary, where the
|
||||
number provably cannot belong to the registered process, an occupied pid
|
||||
preserves the row rather than being argued away by the fencing proof.
|
||||
|
||||
Multiple processes belonging to one legitimate worker cohort are not treated as
|
||||
multiple independent workers: the fleet model from #948/#978 is preserved
|
||||
@@ -182,16 +202,75 @@ problem.
|
||||
|
||||
## Permissions
|
||||
|
||||
* **Allowed:** `controller`, `reconciler`.
|
||||
* **Denied:** author, reviewer, merger — they keep `gitea.read` for diagnosis
|
||||
elsewhere and are refused this surface by role.
|
||||
* The Gitea operation gate stays `gitea.read` because the mutation lands in the
|
||||
**local control-plane worker registry**, not in Gitea — the same model the
|
||||
#601 lease lifecycle uses. **No new Gitea write permission is introduced for
|
||||
any profile**, and no author permission is broadened.
|
||||
Plan and apply are authorized differently, and deliberately so (review 657 B1).
|
||||
|
||||
| | Plan | Apply |
|
||||
| --- | --- | --- |
|
||||
| Capability | `gitea.read` | `gitea.worker_registry.retire` |
|
||||
| Nature | observational; opens no transaction, writes nothing | mutation |
|
||||
| Roles | `controller`, `reconciler` | `controller`, `reconciler` |
|
||||
|
||||
An earlier revision authorized apply with `gitea.read` alone, reasoning that
|
||||
the mutation lands in the local control-plane registry rather than in Gitea.
|
||||
That the write is local makes it **no less a mutation**: sharing an
|
||||
observational permission class with plan meant any profile that could *look*
|
||||
could also *destroy*. Apply now requires its own capability.
|
||||
|
||||
* **Denied:** author, reviewer, merger, and every ordinary read-only profile —
|
||||
they lack the capability, so they fail closed on the permission itself rather
|
||||
than on the role check alone. The role restriction remains as defence in
|
||||
depth: a profile mistakenly granted the capability still cannot reach apply
|
||||
from an author, reviewer, or merger role.
|
||||
* The capability is checked at entry **and** re-resolved immediately before the
|
||||
registry mutation, so a profile change mid-call cannot be outrun.
|
||||
* **No new Gitea write permission is introduced.**
|
||||
`gitea.worker_registry.retire` authorizes exactly one local control-plane
|
||||
transition (`worker_registrations.status -> retired`) and grants no branch,
|
||||
issue, PR, review, merge, or restart authority. No author permission is
|
||||
broadened.
|
||||
* The fleet snapshot remains observational: nothing here turns it into a gate on
|
||||
ordinary author work.
|
||||
|
||||
### Operator step
|
||||
|
||||
No profile holds `gitea.worker_registry.retire` by default, so apply is inert
|
||||
until an operator adds it to the `allowed_operations` of the controller or
|
||||
reconciler profile in `profiles.json`. Removing it again immediately and
|
||||
completely revokes apply, while leaving plan and every other capability
|
||||
untouched. That grant is a configuration change and is outside the scope of the
|
||||
code that implements this feature.
|
||||
|
||||
## External-state fencing
|
||||
|
||||
`BEGIN IMMEDIATE` locks the worker registry and nothing else, so two inputs the
|
||||
decision depends on sit outside the transaction's isolation domain: the
|
||||
workflow-lease table in a separate control-plane database, and OS process
|
||||
liveness. Re-reading them once during revalidation is not sufficient — the
|
||||
per-target loop runs afterwards, so a lease acquired (or a pid revived) after
|
||||
revalidation but before a given row's `UPDATE` would go unnoticed, and the
|
||||
registry-column guard cannot catch it because no registry column changed.
|
||||
|
||||
Two mechanisms close that window, both applied per target immediately before
|
||||
its own write:
|
||||
|
||||
* **`external_fence_fn`** — a version token over active leases
|
||||
(`external_state_fingerprint`), captured inside the transaction *before* the
|
||||
authoritative read and re-compared before every guarded `UPDATE`. Any movement
|
||||
raises, rolling back the whole transaction: once the world has changed, every
|
||||
remaining per-row decision was computed against a world that no longer exists.
|
||||
An unreadable lease store raises rather than returning a token, because
|
||||
"unreadable" must not silently compare equal to "unchanged".
|
||||
* **`liveness_fn`** — a re-probe of process liveness and fencing identity that
|
||||
must affirmatively re-establish that this exact process is gone. It compares
|
||||
`process_start_time`, so a pid number reused since the plan is refused rather
|
||||
than accepted.
|
||||
|
||||
A caller that supplies no `liveness_fn` retires nothing
|
||||
(`liveness_reprobe_unavailable`) rather than proceeding unfenced. The guarded
|
||||
`UPDATE` additionally asserts `host_id`, `boot_id`, and `process_start_time` are
|
||||
unchanged, and all three participate in the CAS token, so fencing movement
|
||||
alone is enough to abort.
|
||||
|
||||
## Non-goals
|
||||
|
||||
* Killing or restarting processes.
|
||||
|
||||
Reference in New Issue
Block a user