reviewing-personhog-protocol

Reviewing personhog protocol changes

Protocol defects survive ordinary review because every component looks correct in isolation — the bugs live in compositions: an authority signal nobody observes during one await, a threshold that coincides with a detector's duration, a rarely-taken exit path that skips the fence. This skill encodes the process that has actually caught them, and the full set of layers a personhog change must clear before it ships.

The process: two passes, then verification

1. Author pass. Sweep the full diff against every lens dimension below. Read final files, not hunks — composed state is where the bugs are.
2. Independent adversarial pass. Spawn a cold-context agent on the full diff. Give it the protocol invariants (references/personhog-invariants.md) and the lens dimensions — never your conclusions or your fixes' rationale, which would anchor it. Demand: ranked findings with file:line, a one-sentence defect statement, a concrete failure scenario (inputs/state → wrong outcome), and a CONFIRMED (traced end-to-end) or PLAUSIBLE (missing check named) label. Ask it to state clean dimensions in one line each — silence is not coverage.
3. Author verification. Verify every finding against code before accepting it. Findings get upgraded, downgraded, or killed with evidence — never adopted on authority. A wrong finding fixed is a new defect introduced.
4. After a fix batch, review again. Fresh agent, full scope, with the fix batch named as the primary attack surface: new code is where new bugs live, and a fix can displace a defect instead of closing it. Ask for a per-finding CLOSED / DISPLACED / STILL OPEN verdict.

The lens dimensions

For each, the question to ask — not a checkbox, a hunt:

1. Authority transitions × in-flight work. Enumerate every writer × key × guard. For each transition of serving authority (acquire, release, fence, deregister): what work is in flight across it, and what guarantees it lands on the right side? Never dismiss a race on likelihood — protocol review argues structure, not odds.
2. Observation latency. A prompt failure signal nobody is listening to is a deferred fence. Enumerate every await a component makes while holding authority (lease, registration, serving state) and ask: is the authority-loss signal raced here, or does this await defer detection for its full duration? Backoff naps, drains, and bootstrap sequences are where this hides.
3. Timescale interactions. Build the full constants table (TTLs, heartbeats, margins, budgets × intervals, timeouts, deadlines, watchdogs, produce/message timeouts) and check pairwise interactions. Two smells: a threshold that equals a detector's duration (the detector's failures become structurally exempt from the threshold), and a margin whose consumer can outspend it (a fence bounded by a timeout larger than the runway). Validate required relations at construction, not in comments.
4. Budget and counter semantics. What resets what, and on which evidence? Progress must mean applied work — never a successful read, which stays available in exactly the wedges budgets exist for. Ask both directions: can a wedge class cycle forever without escalating, and can a transient class escalate spuriously?
5. Lifecycle. Every spawned task joined or aborted on every exit path; raced JoinHandles consumed at most once with every later await site guarded; cancel-by-drop assumptions verified (dropping a loop future drops its owned futures — but never its spawned tasks); teardown ordering stated and tested. The bootstrap window — registered but not yet supervised — is an exit-path zoo of its own.
6. Primitive semantics, verified against implementation. What does the primitive actually do — not what its name suggests? select! short-circuits on first ready arm; FuturesUnordered only progresses when polled; a keepalive response proves a reset at send-processing time, not receipt time; stream end and lease expiry are different facts; etcd answers keepalives for a dead lease with TTL 0, not a closed stream. When in doubt, read the dependency's source.
7. Failure-path parity. The rarely-taken exits — budget exhaustion, timeouts, poison paths, fast-shutdown branches — must uphold the same invariants as the hot path. "The fence exists" is not enough; it must run on every path that drops authority, in the right order relative to deregistration.
8. Escalation legibility. Fail loudly and attributably: a crash cascade that shows up as generic restarts is loud but illegible. Every deliberate escalation should carry the specific cause (which partition, which budget, which margin) in its metric labels and logs.

The layers a change must clear

A personhog protocol change is not reviewed by reasoning alone. Check each layer, in order of cost:

1. Decision-logic coupling (stateright). personhog-stateright model-checks the protocol by driving transitions through the production decision functions (desired_state and friends). If the change touches decision logic, phases, or ack semantics: does the model still compile against it, do the existing properties still hold, and does the change introduce interleavings the model should now cover (a new scenario variant)? Execution-level changes (concurrency structure, supervision) don't need model updates — say so explicitly rather than silently skipping.
2. Protocol integration tests (personhog-coordination/tests/, real etcd): every behavioral change pinned by a test that fails on the old code — see the red-check discipline below. Connection-level behavior (blips, outages, lease margins) is testable by routing the component's store through a byte-forwarding TCP proxy the test controls: sever live connections to simulate a blip, refuse new ones to simulate an outage. The test commons' FlakyProxy provides this (arriving with the etcd-resilience changes); on a tree without it, that is the pattern to build — tokio only, well under a hundred lines.
3. The e2e harness gates (personhog-test-harness gate): run the CI gate scenarios locally against the built tree — at minimum the drain + zombie + writer-lag and kill + scale-up variants, plus any scenario shaped like the change. The gates assert the invariant that matters: every acked write visible in strong reads and Postgres.
4. Mixed-fleet compatibility. Deploys roll pods one at a time: old and new binaries share etcd and the changelog mid-roll. Any change to etcd record shapes, ack semantics, phase meanings, or changelog framing must be read-compatible in both directions across one release, or gated. Also check charts interplay: termination grace periods versus drain timeouts, lease TTLs versus rollout pacing.
5. Observability and the residual ledger. New failure modes need counters with attributing labels and, where they change operator response, dashboard panels. rust/personhog-coordination/README.md is the design + residual-risk ledger: update it when a fix changes a stated guarantee, and never let a known residual silently widen. After deploy, validate on the dev traffic bed: violations zero, restarts flat, the change's own metrics moving as predicted.

Fix discipline

• Red-check every fix: temporarily disable the fix (scratch-copy the file first — never git checkout for temp reverts, it destroys uncommitted work), run the new test, confirm it fails for the predicted reason, restore, confirm green.
• One regression test per fix, at the lowest level that catches it, per /writing-tests. If a fix can't be pinned without heavy new machinery, say so explicitly in the PR rather than silently skipping.
• Fixes to counters, budgets, or thresholds must state their invariant in a comment — the next reviewer checks the invariant, not the arithmetic.

Related

• /writing-tests — the test-worthiness gate for the regression pins.
• /adding-personhog-rpc — the data-plane counterpart (RPCs, storage, routing); this skill owns the coordination plane.
• rust/personhog-coordination/README.md — the protocol's own design and residual documentation; review claims against it.