Repository navigation
feat(personhog): add lifecycle fencing to the leader - #78258
Conversation
Extra distinct ids now always get version 1 mappings instead of consulting posthog_personlessdistinctid for event history. Version 1 is safe either way: the override it emits is a transient no-op when no events exist, and the personless table is being removed. Deletes the claim/revert transaction steps and their queries.
The lifecycle engine drives durable ops generically: create-or-attach by op_id, lease claim/renew/steal by lease_expires_at, a drive loop whose steps commit their work and a CAS step advance in one transaction, plus a config-gated sweeper and GC. The delete driver implements the RFC's started -> marked -> sealed -> unmapped -> completed chain in pre-fence mode: seal from PG version + margin, person tombstone written directly in the unmapped transaction, tombstoned distinct ids recorded in moved. DeletePersons now drives the saga to a terminal step and answers with the recorded outcome; retries with the same op_id attach and get the same answer.
A committed ReleaseFence produced the death document on the request's word alone: with no fence entry (or one the caller installed itself via FencePerson, which verifies nothing), any caller could destroy any person with a fabricated op_id. The committed path now requires a live lifecycle_op_person mark for the op before producing, absorbs retries whose mark already settled as deleted, and fails closed when the mark is absent or unverifiable.
A drained (deposed) leader accepted FencePerson: the fence landed in a map the new owner never consults and was silently discarded at release, leaving the saga holding a seal that protects nothing while the new owner admitted writes above it. fence_person now takes the inflight guard like every other person-mutating path — not for the Kafka HWM (fencing produces nothing) but for ownership currency: the refusal forces the saga's retry onto the current owner, restoring the takeover scan + FencePerson two-source coverage of every mark.
… document A cold leader built the death document with created_at 0, and the writer's upsert projected that epoch-zero timestamp over the row's real creation time — durable corruption on exactly the restart path the request's sealed_version was designed for. The sealed created_at now travels in ReleaseFenceRequest and the death document uses it on warm and cold paths alike, making the re-derivation genuinely deterministic. Also folded in: a committed release now rejects a person_uuid that disagrees with the loaded person (the writer upserts uuid verbatim, so a mismatched request could rewrite the row's identity), and the death version uses checked_add so a hostile sealed_version cannot overflow.
check_fence queried lifecycle_op on every write to a fenced person, while holding that person's lock, on the 5-connection pool shared with every cache-miss load — a hot fenced person could stall unrelated reads pod-wide, and it contradicted the map's whole rationale. The lookup is now pure memory: the leader does not reclaim fences at all, because identity drives every op to a terminal state and a fence outliving a crashed saga is correct, not stale. That guarantee only holds if a release cannot vacuously succeed on the wrong pod, so both fence RPCs now verify this pod serves the partition. The handoff guard alone was not enough: release_partition unfences, so a pod that already handed off looked writable and would have returned OK while the real owner kept the fence, stranding the person.
The scan gates a partition's return to service and reads a set whose size depends on identity's liveness — marks stay live while their op does — so an accumulation of stuck ops could stall or fail every handoff and pod restart. It now runs under a timeout (failing closed: a partition whose fences cannot be known must not serve writes) and reports duration, timeouts, and the active fence count. It also drops the partition's existing entries before installing, so a handoff cancelled after warming and later re-acquired converges instead of accumulating — with the liveness check gone, nothing else would ever clear those entries. Drops the covering-index migration: it optimized a path that runs a few times per rebalance, could not deliver the index-only read it claimed (the join to lifecycle_op needs a lookup per row regardless), and added write amplification to a unique index on the saga's hot write path — in exchange for a non-concurrent rebuild of a uniqueness-bearing index.
CI regenerates these and fails on any diff. No client wrapper or re-export accompanies them: FencePerson and ReleaseFence are called only by the lifecycle saga runner in personhog-identity, and giving Django a one-call person-destroying primitive is not worth the convenience.
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
posthog/personhog_client/proto/generated/personhog/service/v1/service_pb2.py |
0.0% | 31 |
posthog/personhog_client/proto/generated/personhog/types/v1/person_pb2.py |
0.0% | 25–28, 30–106 |
posthog/personhog_client/proto/generated/personhog/service/v1/service_pb2_grpc.py |
37.5% | 252, 258, 522–524, 528–530, 1868, 1898 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 31193827861 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.5% |
8,855 / 22,413 |
warehouse_sources_queue |
█████████░░░░░░░░░░░ 45.9% |
90 / 196 |
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
data_tools |
█████████████░░░░░░░ 66.2% |
90 / 136 |
tasks |
██████████████░░░░░░ 70.4% |
35,223 / 50,047 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
█████████████████░░░ 82.5% |
27,210 / 32,964 |
apm |
█████████████████░░░ 83.8% |
1,220 / 1,455 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,649 |
notebooks |
█████████████████░░░ 86.4% |
8,029 / 9,296 |
data_modeling |
█████████████████░░░ 86.6% |
8,451 / 9,761 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
cohorts |
██████████████████░░ 87.6% |
6,482 / 7,400 |
managed_warehouse |
██████████████████░░ 87.8% |
5,928 / 6,752 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
data_warehouse |
██████████████████░░ 88.0% |
11,552 / 13,130 |
business_knowledge |
██████████████████░░ 89.0% |
4,384 / 4,928 |
engineering_analytics |
██████████████████░░ 89.2% |
6,725 / 7,537 |
exports |
██████████████████░░ 89.3% |
7,484 / 8,378 |
dashboards |
██████████████████░░ 89.4% |
6,001 / 6,710 |
visual_review |
██████████████████░░ 89.5% |
5,870 / 6,558 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
conversations |
██████████████████░░ 90.5% |
18,337 / 20,256 |
canvas |
██████████████████░░ 90.6% |
2,080 / 2,296 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.1% |
11,135 / 12,225 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
slack_app |
██████████████████░░ 91.6% |
10,589 / 11,565 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
notifications |
███████████████████░ 92.6% |
1,017 / 1,098 |
mcp_analytics |
███████████████████░ 92.7% |
3,810 / 4,112 |
ai_observability |
███████████████████░ 92.9% |
17,116 / 18,426 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
marketing_analytics |
███████████████████░ 92.9% |
14,833 / 15,967 |
mcp_store |
███████████████████░ 92.9% |
6,952 / 7,482 |
web_analytics |
███████████████████░ 93.1% |
15,963 / 17,154 |
surveys |
███████████████████░ 93.1% |
5,858 / 6,290 |
posthog_ai |
███████████████████░ 93.3% |
1,327 / 1,423 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
product_analytics |
███████████████████░ 93.5% |
7,027 / 7,517 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
legal_documents |
███████████████████░ 93.8% |
1,628 / 1,736 |
workflows |
███████████████████░ 94.3% |
7,951 / 8,436 |
endpoints |
███████████████████░ 94.3% |
8,771 / 9,306 |
tracing |
███████████████████░ 94.4% |
2,678 / 2,837 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.8% |
3,478 / 3,669 |
messaging |
███████████████████░ 95.4% |
3,401 / 3,565 |
replay_vision |
███████████████████░ 95.7% |
18,103 / 18,923 |
experiments |
███████████████████░ 95.7% |
27,871 / 29,115 |
logs |
███████████████████░ 95.8% |
11,623 / 12,135 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
growth |
███████████████████░ 96.4% |
4,937 / 5,123 |
feature_flags |
███████████████████░ 96.4% |
17,580 / 18,238 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
access_control |
███████████████████░ 96.9% |
870 / 898 |
customer_analytics |
███████████████████░ 97.1% |
10,838 / 11,160 |
warehouse_sources |
███████████████████░ 97.5% |
380,188 / 390,037 |
data_catalog |
████████████████████ 97.9% |
2,676 / 2,734 |
analytics_platform |
████████████████████ 98.1% |
2,254 / 2,298 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
✅ Django migration risk — no migrations to analyze
No Django migrations need risk analysis.
|
Reviews (1): Last reviewed commit: "chore(personhog): regenerate python prot..." | Re-trigger Greptile |
|
|
||
| match outcome { | ||
| ReleaseOutcome::Committed => { | ||
| if req.sealed_version <= 0 { |
There was a problem hiding this comment.
Guarding against version 0 here is a bit odd and I presume is supposed to work in conjunction with rust/personhog-leader/src/pg.rs:113 which sets missing versions to 0.
Technically, a person at version 0 is possible (if very fleeting, would have to cold load a person stub onto a leader). As the code stands, we will happily fence these stub person records, but then this guard we doesnt allow them to be unfenced.
We should probably stop setting a missing person version in the pg.rs code as 0, then we wouldn't need this guard here that coincidentally lumps in person stubs and persons with missing versions as the same thing
There was a problem hiding this comment.
Good catch — fresh stubs are genuinely inserted at version 0 (stub_create.rs), so this guard would ghost-fence them once exact sealing lands. Fixed in 09d0c3e: sealed_version is now explicitly optional in the proto, so presence is checkable without the 0 sentinel and a seal of 0 releases fine (regression test pins it). I kept the pg.rs unwrap_or(0) as-is: it matches the COALESCE(version, 0) + 1 convention used everywhere else (writer guard, stub revival), and with the sentinel gone, 0 is a legitimate version end to end.
| //! | ||
| //! One known bound: a takeover that runs between a release being acked | ||
| //! and the saga settling its mark row installs a fence for an op that is | ||
| //! already done. The next partition movement or restart clears it. |
There was a problem hiding this comment.
Is this acceptable?
Could we add a check (could be non-blocking) after a write to a particular person are rejected by a fence that reads postgres to understand if the op id for that fence has completed? I think that would unblock these ghost fenced persons much sooner, and only require a non-blocking PG read. that read can even look up the op with the op_id because it exists in the fence map.
There was a problem hiding this comment.
I understand it creates a PG in the write path (rather than the requirement of only having this added to the warming phase), but this would only be for write paths when a write is rejected due to a fence and we can also evaluate it lazily.
might be worth it to keep us from rejecting writes for persons caught by this until the partition changes hands
There was a problem hiding this comment.
Done in 07c2738 — added a lazy healer along these lines: a write rejected by a fence triggers a non-blocking mark-row read (looked up by the op_id from the fence map, exactly as you suggested), and a fence whose op has settled is dropped, so the caller's retry goes through instead of waiting for the partition to change hands. Fail-closed: removal only happens on Postgres's word and only if the entry still belongs to the checked op (remove_if on op_id), and checks are debounced per person with a 5s cooldown so a write storm against a legitimately fenced person costs at most one point read per cooldown. The first rejected write still bounces (the check is off the request path per your follow-up); the retry lands.
There was a problem hiding this comment.
Basically objective here was not to make this sync and have a way to eventually recover, felt like the best compromise
| //! pod serves the partition, so a misrouted call fails and identity's | ||
| //! retry reaches the pod whose map actually gates the writes. | ||
| //! | ||
| //! The map is NOT a cache: it has no capacity limit and no eviction path |
There was a problem hiding this comment.
i think it could be a good safety fallback to have a hard cap on index size here, and provide back pressure to the sagas if we're hitting it. this would protect us from a huge surge from a big customer
There was a problem hiding this comment.
Added in d0ae576: fence_map_max_entries config (default 250k, ~25 MB — a memory fuse, not a tuning knob). At the cap, new FencePerson calls shed with RESOURCE_EXHAUSTED, which the saga's retries absorb as backpressure. Two exemptions, both deliberate: same-op re-seals (the person is already fenced, refusing frees nothing) and the takeover scan (those marks are already live, so the freeze must hold — if the live-mark set is truly huge the scan's 10s timeout already fails the handoff, which is the right failure mode).
| LIFECYCLE_OP_TYPE_MERGE = 2; | ||
| } | ||
|
|
||
| message FencePersonResponse { |
There was a problem hiding this comment.
can this ever fail? do we need a way to indicate status if so?
There was a problem hiding this comment.
It can fail, and always through the gRPC status rather than in-band: FAILED_PRECONDITION with x-person-fenced metadata when another op holds the person, NOT_FOUND when the person is destroyed, FAILED_PRECONDITION when this pod doesn't serve the partition (the saga's retry re-routes), and — new with the cap thread above — RESOURCE_EXHAUSTED when the fence map is at capacity. OK is all-or-nothing: fence installed + sealed state returned. An in-band status field would duplicate the gRPC status and invite "OK but failed" ambiguity, so I documented the full failure surface on the message instead (4c27804).
| partition, | ||
| reason: format!("fence takeover scan failed: {e}"), | ||
| })?; | ||
| if installed > 0 { |
There was a problem hiding this comment.
might be nice to have a warming specific metric here on how much we install / read
There was a problem hiding this comment.
Added in a2a5b0a: personhog_leader_fence_scan_rows_read_total and personhog_leader_fence_scan_fences_installed_total, incremented inside the scan where both numbers are known. Read next to installed is the scan's amplification — it reads the whole live-mark set and keeps ~1/num_partitions of it — so that ratio growing is the signal the scan-the-world design needs revisiting.
| // The death document closes the person's stream: drop the | ||
| // entry; a later read recovers the death record via the | ||
| // dirty mark and answers not-found. | ||
| self.cache.remove(partition, &cache_key); |
There was a problem hiding this comment.
is it worth keeping in the cache with an explicit delete field? so we dont have to fallback if a subsequent attempt comes in
There was a problem hiding this comment.
You're right, and it turned out the removal was self-defeating: the next attempt recovered the death document from the changelog and re-installed it into the cache anyway (recover_or_load puts whatever it recovers), so evicting only cost an extra Kafka fetch — and once the dirty mark was pruned, every post-death attempt fell through to an uncached PG read, forever. Fixed in 7106385: the death document stays cached (commit_document already put it there) and answers post-death reads and writes with an authoritative not-found from memory, matching what the recovery path already did deliberately. One residual for a follow-up: a revived person (revival reuses the same person id) stays masked by the cached death doc until the entry leaves the cache — that staleness is pre-existing and identical in both designs, since recovery re-installs the death doc either way.
| // The version sealed at fence time. The death document is produced at | ||
| // sealed_version + 1. Required when outcome = COMMITTED. | ||
| int64 sealed_version = 6; | ||
| // The person's creation time (epoch millis) as sealed at fence time. |
There was a problem hiding this comment.
nit: my bot says that this will actually be seconds, not millis based on how we read and write from pg. that seems like the wrong choice. this is just a reminder for us, but a follow up we'll need to do at some point is to audit how we do tiemstamping across the entire system and make sure its consistent with each other
There was a problem hiding this comment.
Confirmed — it's epoch seconds end to end: the leader loads created_at.timestamp() and the writer binds via epoch_secs_to_datetime. Fixed the field doc in 3be4f03. Agreed on the system-wide timestamping audit as a follow-up; new_person_created_at_ms in the same proto file really is millis, so the inconsistency is already visible within one file.
| #[tokio::test] | ||
| async fn writer_projects_death_documents_as_tombstones() { | ||
| let pool = create_test_pool().await; | ||
| let team_id: i32 = 99_010; |
There was a problem hiding this comment.
nit: concurrently running tests using the same team id i think will clean up each others rows with the cleanup_team function, potentially stamping on one another and making this flaky
There was a problem hiding this comment.
Right — and it was worse than a maybe: team 99_010 was shared by two tests, so their cleanups could delete each other's rows mid-flight. Fixed in 7c4de53 (and 2dc89de for the leader's fence suite, which had the same disease via shared team 1): every test now mints a unique team id (per-run salt + counter), so the shared tables are disjoint per test, leftover rows from failed runs can't collide, and the leading cleanup calls became no-ops and are gone.
|
😎 Merged successfully - details. |
A fresh person stub is created at version 0, so a committed release carrying sealed_version 0 is legitimate. The guard used 0 as an unset sentinel, which fenced stubs forever. sealed_version is now explicitly optional in the proto, making presence checkable without a sentinel.
A takeover between a release ack and the saga settling its mark row installs a fence no ReleaseFence will ever clear, freezing the person until the partition next changes hands. A write rejected by a fence now triggers a non-blocking mark-row read; a fence whose op has settled is dropped, so the caller's retry goes through. Fail-closed: the fence only drops when Postgres says the mark is no longer live, per-person checks are cooldown-bounded, and a newer op's fence is never touched.
The fence map has no eviction, so a surge of lifecycle ops from one customer could grow it without bound. FencePerson now sheds new fences with RESOURCE_EXHAUSTED at fence_map_max_entries (default 250k, ~25 MB), backpressure the saga's retries absorb. Re-seals of already-fenced persons and the takeover scan are exempt: those fences exist already, and refusing them would free nothing while breaking the freeze.
Evicting it only re-derived it: the next attempt recovered the death record from the changelog and re-installed it, and once the dirty mark was pruned every attempt fell through to a PG read. The cached is_deleted entry answers post-death reads and writes with an authoritative not-found from memory, matching the recovery path.
Tests shared team 1, so concurrent runs collided on the live-mark unique index and a failed run's leftover marks broke the next one. A per-harness unique team id makes the Postgres state disjoint per test with no cleanup ordering to get right.
Two tests shared team 99_010, so their cleanup_team calls could delete each other's rows mid-flight when run concurrently. A per-test unique team id makes the shared table disjoint per test; the leading cleanups became no-ops and are gone.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| return Err(Status::not_found("person is destroyed")); | ||
| } | ||
|
|
||
| self.fences.insert(cache_key, FenceState { op_id, op_type }); |
There was a problem hiding this comment.
Medium: Unverified callers can freeze arbitrary persons
FencePerson installs a caller-supplied op_id without verifying a live lifecycle mark or an authenticated saga-runner identity. Any client able to reach the shared PersonHog router can repeatedly fence a chosen team/person and deny its property writes; the aborted ReleaseFence path is similarly authorized only by the supplied UUID. Require authenticated service identity for both RPCs, or verify the matching live non-target mark before installing or releasing a fence.
PR overviewThis PR adds lifecycle fencing to the PersonHog leader, including RPCs to install and release per-person fences and support for restoring fence state during partition ownership changes. The current implementation has significant authorization and recovery gaps: reachable clients can install or release fences using caller-controlled operation data, enabling denial of writes or premature reopening during lifecycle operations. Fence state can also fail to survive leader restart or partition reassignment under the default database configuration, while concurrent requests can exceed the intended memory limit. All four identified issues remain open, so the security posture still requires substantial hardening. Open issues (4)
Fixed/addressed: 0 · PR risk: 7/10 |
…nter nextest runs each test in its own process, so the per-process counter with a seconds-based salt handed the same team id to every test launched in the same second — two fence tests collided on the live-mark unique index in CI. A random id per call needs no cross-process coordination.
|
🤖 Bot comment (Claude Code, session directed by @nickbest-ph). Context: I built the MergePersons stack on top of this branch (#79225–#79228) and the adversarial review pass surfaced two things that belong to this PR rather than that stack: 1. Flaky test: 2. Sealed/death versions never consult the emitted-versions floor |
|
@nickbest-ph Think the |
A pre-fence write with an indeterminate outcome leaves a version spent above the cache's; sealing below it let the death document land at a version that may already be live. FencePerson's seal and the committed release's death version now consult the emitted-versions floor. Also fix fence test flakiness: the at-capacity test wiped its first seeded person by recreating a shared partition, and the harness cache was sized at 100 bytes.
| } | ||
| } | ||
|
|
||
| match outcome { |
There was a problem hiding this comment.
Low: Caller-controlled lifecycle releases
A client can obtain the active op_id from x-person-fenced-op-id, then submit ABORTED to remove the fence without any mark verification, reopening writes during the operation. The COMMITTED branch also accepts a marked row of any operation type and trusts caller-supplied seal fields, allowing a premature death document. Restrict this RPC to the saga runner and derive the outcome and sealed values from lifecycle_op and lifecycle_op_person; committed releases should require a delete operation with a sealed mark, while aborted releases should verify that the operation actually aborted.
| kafka_producer.clone(), | ||
| config.ingestion_warnings_topic.clone(), | ||
| ); | ||
| let fence_scan_pool = fallback.as_ref().map(|f| f.pool.clone()); |
There was a problem hiding this comment.
Medium: Fences fail open when PG fallback is disabled
fallback_database_url defaults to empty, but FencePerson remains enabled while this disables the lifecycle-mark scan. After the partition is reassigned or its leader restarts, a client can submit a write above the sealed version because the new owner warmed without reconstructing the fence. Use a required lifecycle database pool independent of the optional read fallback, or refuse fencing and partition acquisition when that pool is unavailable.
| // person is already fenced, refusing frees nothing — and so is | ||
| // the takeover scan, whose marks are already live. The saga's | ||
| // retry absorbs the backpressure. | ||
| if !refence && self.fences.len() >= self.fence_map_max_entries { |
There was a problem hiding this comment.
Low: Concurrent fence requests can bypass the map capacity
The length check does not reserve a slot, and it occurs before lookup_or_load_locked can await Postgres or changelog recovery. A client can concurrently fence many uncached persons so every request observes space before any request inserts, allowing the map to grow well beyond the configured memory fuse. Reserve capacity atomically before the awaited load and release that reservation when the request fails.
…nhog-leader-fence # Conflicts: # posthog/personhog_client/proto/generated/personhog/types/v1/person_pb2.py # rust/personhog-leader/src/cache/partitioned.rs # rust/personhog-leader/src/cache/persons.rs # rust/personhog-leader/src/pg.rs # rust/personhog-leader/src/service.rs # rust/personhog-leader/tests/common/mod.rs # rust/personhog-writer/src/pg.rs
Problem
The delete saga needs a way to freeze a person while it destroys them.
Without one it runs in "pre-fencing mode": it guesses a seal by adding a margin of 100 to the current version and hopes no writer catches up.
That is a guess, not a guarantee, and a write that lands after the seal is silently destroyed by the delete.
This PR lands the leader-side half of lifecycle fencing from the personhog identity saga RFC.
It stacks on #77360 and has no caller yet: the saga still runs pre-fencing until stage 6 wires it up.
Changes
FencePersonfreezes a person under the same per-key lock that admits writes and returns the sealed state in one response, so there is no gap for a write to slip through.While fenced, writes fail
FAILED_PRECONDITIONwith anx-person-fencedmetadata key carrying the op type. Reads are unaffected.ReleaseFencecloses it. Committed produces a death document to the changelog (is_deleted = trueatmax(sealed, current) + 1), evicts the cache entry, and drops the fence. Aborted just drops the fence and the person resumes normal life.The writer projects
is_deletedonto the person row through the existing batched upsert, so a tombstone is an ordinary versioned update rather than a row delete. The version guard keeps a lagging record from resurrecting it.Fences live in an in-memory map per leader. Postgres is read in exactly one place: a takeover scan at partition acquisition, before the partition accepts writes. The leader never reclaims a fence on its own, because personhog-identity drives every op to a terminal state, so a fence outliving a crashed saga is correct rather than stale.
Note
Both fence RPCs verify this pod actually serves the partition. Without that, a misrouted release removes nothing, returns OK, and leaves the real owner's fence standing while the saga believes it released, freezing that person with no retry coming.
Also here: the test harness grows a fence chaos window (
--fence-after/--fence-release-after) whose journal flags any write acked above the seal, and the Python proto stubs are regenerated because CI fails on a diff. No Python client wrapper accompanies them. These RPCs are saga-runner only, and a one-call person-destroying primitive in Django is not worth the convenience.Node stubs are deliberately left alone. Regenerating them surfaced pre-existing drift unrelated to this change (three RPCs and a
batch_sizefield already in the proto but missing from the committed stubs), and there is no CI gate on them. That cleanup belongs in its own PR.How did you test this code?
Automated only. I have not run this against a live stack. The saga has no fence caller yet, so there is no end-to-end path to exercise by hand.
New leader integration tests in
rust/personhog-leader/tests/fence_integration.rs, each named for a regression no existing test covered:op_iddestroying an arbitrary personFencePersonis refused while the partition is handoff-fencedWriter integration test covering tombstone projection and a lagging record failing to resurrect it. Harness journal unit test for the fenced-write invariant.
cargo test -p personhog-leader -p personhog-test-harness -p personhog-commonis green at 177 tests, withcargo fmt,cargo clippy --workspace --all-targets --all-features -D warnings, andcargo shearclean.Automatic notifications
Docs update
Not user-facing. Internal RPCs with no caller yet.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I wrote the feature, then had Claude run
/review-codeover it with six specialist reviewers. We walked the findings one at a time. Five became commits, four I rejected after we dug into them.The interesting one was the write path. Claude's first proposal for the per-write Postgres liveness check was a TTL cache, which I pushed back on: the leader should read Postgres once at takeover and otherwise trust its map. That was right, and it changed the design rather than just the code. The lazy liveness check is gone,
check_fenceis a plain synchronous lookup, and reclamation is identity's job. The ownership guard exists to make that model hold. It is also what deleted the shared-connection-pool contention the perf reviewer flagged, instead of merely reducing it.Rejected, with reasoning: a deploy-order tombstone hazard that cannot fire because writer and leader ship together here and the saga has no caller; the fence
op_idin rejection metadata, which was rated critical against an untrusted-caller model we do not have, and which is a genuinely useful debugging breadcrumb for an internal service; "fencing is silently optional", which cannot be silent because a leader missing that config fails every cache miss loudly; and the router's stash drain flattening the fence metadata, which is latent, narrow, and better fixed when something actually consumes the signal.I also dropped a covering-index migration that was in the original branch. It optimized a path that runs a few times per rebalance, could not deliver the index-only read it claimed because the join needs a lookup per row anyway, and added write amplification to a unique index on the saga's hot write path, in exchange for a non-concurrent rebuild of a uniqueness-bearing index.
Tools: Claude Code (Fable 5). Skills invoked:
/review-code.