fix(connectors): the external feed sync reads membership before it writes (TASK-164) - #1952
Merged
Merged
Conversation
…ites (TASK-164) `syncExternalFeeds` read no membership at all. The flag-on path wrote posts AS `integration.createdBy`, and the default path handed the owner's feed to the pod's curator agents; `createMessage` would 403 that same owner. Found by Wren (74669) while closing TASK-161, and kept out of #1940 because it needs a read plus a pause rule rather than a predicate swap. The read goes at the top of the per-integration body, before `syncRecent`, so a departed owner makes no provider call and advances no cursor either. The predicate is `isListedPodMember` - the same rule the pod's own write paths implement - so the feed is never more permissive than the pod it writes into. A pod that is gone pauses too: there is no surface left to write into, and syncing into nothing looks like a healthy run. Pause shape (wren, TASK-164): `status: 'error'` with a Commonly-written `errorMessage` and `errorMessageUserFacing: true`, written as its own update rather than thrown into the catch, which stamps the flag false. `isActive` stays true, so the row stays on the owner's Connectors page - that page has no action for x/instagram, which is why the copy carries the next step itself. `'error'` also takes the row out of the sync query (`status: 'connected'`), so the pause is written once per connection rather than every tick; a re-saved row pauses again on the next sync, and no resume logic is added. Also, admin rows would have paused at birth: only the FIRST requester of the Global Social Feed pod became a Mongo member (at creation), and a second admin configuring the other feed type was mirrored into PG alone. `ensureGlobalSocialFeedPod` now `$addToSet`s the requester into Mongo `members` and hands on the refetched pod. Not in scope, noted by wren: those global routes find their pod by name, and pod names are not unique.
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
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.
Closes the exception #1940's body names: every other connector site reads the strict predicate, and this one read nothing.
The gap
syncExternalFeedsloaded integrations and went straight to the provider. No membership read anywhere in the service, so it could not tell a creator from an ordinary departed member:EXTERNAL_FEED_PERSIST_POSTS=1) wrote posts asintegration.createdBy;appendIntegrationBuffer+enqueueCuratorEvents) handed that owner's feed to the pod's curator agents.createMessage403s that same owner. The owner can stop being a member without anyone touching the integration —leavePodfilters them out ofmembers, agent cleanup$pulls them — and the row kept syncing as them. Found by Wren (74669) while closing TASK-161.The fix
The read goes before the provider call, at the top of the per-integration body:
isListedPodMember(pod, integration.createdBy), the same predicate the pod's own write paths implement. A departed owner therefore makes no provider request, advances no cursor, and writes nothing on either path — buffering and curator events are both behind it.A pod that is gone pauses too. It cannot name a member, and the alternative (sync into nothing, report success) is the failure mode the row is about. That one is my addition beyond the row's text, flagged here rather than buried.
Pause shape, per Wren:
status: 'error'with a Commonly-writtenerrorMessageanderrorMessageUserFacing: true, written as its own update rather than thrown into the catch — the catch stampserrorMessageUserFacing: false, which is right for provider text and wrong for copy a person must read.isActivestays true so the row stays on the owner's Connectors page; that page has no action for x/instagram (integrations.ts:681), which is why the copy carries the next step itself.'error'also takes the row out of the sync query (status: 'connected'), so the pause is written once per connection rather than every tick, and a re-saved row pauses again on the next sync. No resume logic is added.Admin rows would have paused at birth, which is why this PR also touches the admin route: only the first requester of the Global Social Feed pod became a Mongo member (at pod creation), and a second admin configuring the other feed type was mirrored into PG alone (
ensureGlobalPodPostgresSync).ensureGlobalSocialFeedPodnow$addToSets the requester into Mongomembers— the surface the predicate reads and the pod's own write paths enforce — and hands on the refetched pod.createdByis untouched: it is the row's owner, not a membership record.Consumers:
schedulerService(two call sites) and the admin/syncroute. Both get the same behaviour; the paused entry comes back aspaused: true, success: falsewith the reason ascontent.Witnesses (5 new arms, 23 tests in the two suites)
registry.getnot called,Post.insertManynot called, no curator event, no$pushto the buffer, one write:status: 'error'+errorMessageUserFacing: true+ the reason,isActiveuntouched, resultpaused: truePost.findnot called,Post.insertManynot called, no provider callpausedundefined$addToSetwritten, pod refetched, the new row'screatedByis that adminupdateOne, no refetchLedger — 10 mutations, each alone, 9 no-survivor
status)integration._id)members.includes(owner)instead of the shared predicate!podclause droppedM2 is a tripwire, not coverage.
isListedPodMemberalready refuses a null pod, so behaviour cannot distinguish the two forms. I kept the clause because relying on a util's nil handling for a pod lookup couples this branch to that implementation detail, and the pod-gone arm reads better for it.M7 and M10 both redden the controls rather than the departure arms, and that shape is the point. A wrong principal, or reference equality instead of value comparison, pauses everyone — so the arms that can see it are the ones asserting a listed owner is not paused. The departure arms pass under both for the wrong reason, and saying so is more useful than a count.
M10 is why the fixture changed. It survived the first ledger because my pod stub and my integration fixture shared one
ObjectIdinstance, soincludespassed by reference.mockPodMembersnow stores a fresh instance carrying the same hex, which is what a lean read produces — the arm would otherwise have certified a fixture rather than the code. This is theTESTING.mdfixture-shape rule one layer over: the mutation did not fail to apply, it applied and was admitted by the instrument.Verification
__tests__/unit/services+__tests__/unit/routes— 300 suites / 2817 tests green.services/externalFeedService.tsandroutes/admin/globalIntegrations.ts0 errors, 0 on touched lines. The two test files are at theirmainbaseline (16 and 12import/*errors, the repo-wide pattern for.jstests requiring.tsmodules, not gated); the newPodrequire carries that same pair. Twoobject-curly-newlineviolations I introduced by copying the surrounding one-linebody: {...}style are fixed —mainhas one such pre-existing violation atadmin.globalIntegrations.test.js:243, untouched.