feat(personhog): route ingestion person writes by store mode - #81624
Open
z0br0wn wants to merge 6 commits into
Open
feat(personhog): route ingestion person writes by store mode#81624z0br0wn wants to merge 6 commits into
z0br0wn wants to merge 6 commits into
Conversation
PERSONS_STORE_MODE selects the world person writes land in: pg keeps today's path untouched, personhog routes allowlisted teams to the identity and leader RPCs, and shadow runs the personhog verb after the authoritative Postgres call, counting failures without failing the batch. Shadowed writes re-resolve the personhog world's own person by distinct id, because Postgres row ids mean nothing there. Merge execution, deletes, and lifecycle marks stay on Postgres in every mode until the merge saga owns them, as does any verb under a Postgres transaction. A bad mode or a missing endpoint fails boot loudly.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Contributor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
PR overviewThis pull request routes ingestion-time person writes according to the configured store mode, including support for the Personhog-backed persons store. Two security issues have been addressed, but one denial-of-service path remains open. A user with a valid project token can submit a merge-producing event that causes Personhog-mode ingestion to return retriable errors, repeatedly blocking the affected worker from advancing. Open issues (1)
Fixed/addressed: 2 · PR risk: 5/10 |
Review found that a personhog-routed team's merge fetched persons from the personhog world and then mutated Postgres rows keyed on those ids, whose sequences are independent — wrong-row writes on collision. Two mechanisms allowed it: the routing store delegated inTransaction to the Postgres store, whose transaction wrapper wraps itself, so nothing inside a merge transaction ever routed again; and the personhog store implemented no interface, so its missing merge members were invisible and routing silently delegated them to Postgres. The personhog store now declares PersonsStore, with the compiler enforcing completeness: merge mutations are the merge saga's loud placeholders, cohort bookkeeping keeps its deliberate no-ops, and the vestigial batch-bound class the pipeline never reached is deleted. The routing store builds the transaction wrapper around itself, so transactional verbs re-enter routing, and merge verbs route to the team's world and nowhere else — a routed team's merge fails at the store placeholder before any Postgres mutation. Store mocks are now compile-checked against the interface, so drift breaks the build instead of surviving as stale mocks.
Two review-driven simplifications. Merge verbs stop being a special family: they route through the same combinator as every other verb, so personhog mode reaches the store's pending-saga placeholders, shadow mode shadows them like anything else (the placeholder throw lands in the existing swallow-and-count machinery), and implementing the saga changes zero routing lines. The per-team allowlist is gone: PERSONS_STORE_MODE applies to the whole deployment. Per-team sampling coupled unrelated worlds inside one Postgres transaction; with a deployment-wide mode, inTransaction routes by mode like any verb — pg and shadow run the real Postgres transaction, personhog mode answers with the store's placeholder — and the repository-backed transaction wrapper machinery deletes.
The routing comments still described the removed per-team allowlist, the merge special-casing, and signature drift the interface work eliminated.
fetchPersonsForUpdateByDistinctIds still pinned to Postgres with a stale merge comment, deletePersons kept an empty-array short-circuit that existed only for the removed per-team guard, and flush stats reported the Postgres world alone. All three now follow the uniform routing, and route() drops the team parameter the allowlist removal left dead.
flush() serializes passes and claims a snapshot of every lane up front, so ops folded mid-pass ship on the next pass instead of mutating an entry already in flight. A failed ship restores its entry, so a sibling batch's unacked ops survive another batch's flush failure. A no-change lane ships instead of being suppressed when a sibling batch holds ops for the same person, whose baseline the verdict never saw. Also: pending-saga placeholders reject instead of throwing synchronously, the routing store drops its dead transaction rule and routes personPropertiesSize/prefetchPersons/flush uniformly, and getFlushStats no longer double-counts batches in shadow mode.
| * never delegates this member. Reaching it is a wiring bug. | ||
| */ | ||
| inTransaction<T>(_description: string, _transaction: (tx: PersonsStoreTransaction) => Promise<T>): Promise<T> { | ||
| return Promise.reject(new PersonhogPendingRpcError('inTransaction', 'merge saga')) |
Contributor
There was a problem hiding this comment.
Low: Merge events halt personhog-mode ingestion
An attacker with any valid project token can send a merge-producing event such as $merge_dangerously or an $identify joining two distinct IDs. The merge service calls inTransaction, so this rejection bubbles through /ingest as a retriable 500 and marks the server fatal; redelivery of the same event prevents the worker from advancing. Keep authoritative personhog mode unavailable until merges are implemented, or ensure these events are handled without poisoning the batch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The personhog persons store merged with no way to receive traffic: ingestion builds only the Postgres store, so the identity get-or-create, leader fold, and strong-read paths run untested against real event shapes.
Changes
PERSONS_STORE_MODEselects the world person writes land in, deployment-wide:pg(default) builds nothing new,personhogroutes every verb to the personhog store,shadowruns the Postgres call as the authoritative result and the personhog call after it, counting failures without failing the batch.PersonhogPersonsStorenow declaresimplements PersonsStore, so the compiler enforces member completeness. Its custom batch-bound wrapper was unreachable from the pipeline and is deleted; the generic wrapper serves it like any store.PERSONHOG_ADDR/PERSONHOG_IDENTITY_ADDR, fails boot with one error naming the knob.personhog_store_shadow_errors_total{verb},personhog_store_shadow_skips_total{verb}.How did you test this code?
implements.jest.Mocked<PersonsStore>factory, so interface drift breaks the build instead of leaving stale mocks.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Skills invoked:
/writing-tests,/writing-code-comments,/writing-pr-descriptions.The design converged through review rounds directed by Zach. Review found personhog-world row ids reaching Postgres merge mutations; the fix went through several rejected shapes (routing-layer guards, event pinning, per-verb special-casing) before landing on the final principle: the store carries loud placeholders for unimplemented capability, routing treats every verb uniformly, and the per-team allowlist was removed because per-team sampling coupled unrelated worlds inside one Postgres transaction. A reviewer also flagged untyped store mocks, which became the compile-checked factory.