✨ Add modEventDivertBlobs event to send blobs to abyss #2238

foysalit · 2024-02-29T00:25:23Z

This PR adds a new mod event that sends all linked blobs to a record to an external service for performing further analysis on the blobs.

The new event expects an array of subjectBlobCids that will be forwarded to the external service.

A couple of open questions

Do we need to validate the event for subjectBlobCids or default to ALL blobs in the record if none are specified?
What's the "right way" of getting the blob?

…e-review-state

foysalit · 2024-02-29T00:26:31Z

packages/ozone/src/daemon/blob-diverter.ts

+  }
+
+  private async getBlob(opts: { did: string; cid: string }) {
+    // TODO: Is this safe to do or should we be reaching out to the pds instead?


Need some help here figuring out the right way to get the blob.

@devinivy might be able to fill in with typescript-specific tricks... maybe there is a helper for this already? You could look in the appview code, in the auto-labeler section.

The general flow is to resolve the DID doc (probably from a local cache, or even just passing the full doc through to this method from elsewhere); get the account's PDS from there; make an un-authenticated request to the getBlobs endpoint, with appropriate HTTP retries and timeout. For correctness, then verify the hash of the content bytes against the blob CID.

foysalit · 2024-02-29T00:28:09Z

packages/ozone/tests/blob-divert.test.ts

+    status: number,
+    data: Record<string, boolean>,
+  ) =>
+    nock(BLOB_DIVERT_SERVICE_HOST)


We don't use this pattern of intercepting http reqs anywhere in the codebase so wondering if it's ok to introduce this here?

I often avoid these kinds of "live" mocking libraries because they tend to create some weird interactions. I think we can give it a whirl and if it shows any signs of weirdness, not have any qualms about ripping it out.

It seems like we could pretty easily mock it in the application by placing a method on BlobDiverter responsible for getting this response, and instead just mock-out that method. Node now comes with pretty awesome built-in mocking which makes this quite easy to do: https://nodejs.org/api/test.html#class-mocktracker

cool ended up using the jest.fn mocks.

bnewbold · 2024-02-29T02:06:47Z

Some high-level comments:

I think it could be just modEventDivert, which could in theory make it more generic in the future? but i'm also fine with modEventDivertBlobs
I think modEventDivertBlobs should take a list of blob CIDs, instead of fetching and sending all. similar to how we do blob purges by selecting. a different (in UI) will be having it default to "all" images.
I kind of assumed that we would also purge the blob CIDs at the same time, but I guess to make it work better with un-takedown (if the content is reviewed and found to be ok), we would want to revert. I guess we could either make the "un-takedown" event work with content that had been diverted; or have the UI action first a divert, then a second takedown (with the same blobs purged). Doing it all in the divert action feels more robust to me in some ways, but splitting in to multiple events has also become something of a pattern for ozone API.

foysalit · 2024-02-29T11:38:19Z

I think it could be just modEventDivert, which could in theory make it more generic in the future? but i'm also fine with modEventDivertBlobs

I'm also fine with modEventDivert and agree that the generic wording might be helpful down the line.

I think modEventDivertBlobs should take a list of blob CIDs, instead of fetching and sending all. similar to how we do blob purges by selecting. a different (in UI) will be having it default to "all" images.

Yep, that's what happens right now, we expect the event to send us blobCids but I was asking if we should validate that there in fact, at least 1 blobCid and if not, should we fail or assume that all blobs linked to the record should be diverted?

Doing it all in the divert action feels more robust to me in some ways, but splitting in to multiple events has also become something of a pattern for ozone API.

Yeah I'd go for multiple event approach but just let the backend emit a consecutive takedown event when divert is received.

…e-review-state

bnewbold

From a high level, looking great!

devinivy · 2024-03-04T15:33:35Z

packages/ozone/src/db/schema/blob_divert_event.ts

+  subjectDid: string
+  subjectBlobCid: string
+  subjectUri: string
+  divertedAt: Date | null


I think we can name this confirmedAt to keep it consistent with repo_push_event, record_push_event, blob_push_event. The idea is that once the event has been accepted by the other service it's "confirmed."

devinivy · 2024-03-04T15:51:35Z

packages/ozone/src/daemon/blob-diverter.ts

+      return false
+    }
+
+    const url = `${this.serviceConfig.url}?did=${subjectDid}&uri=${subjectUri}`


We will want to ensure these parts get %-encoded. The most reliable way to do it is probably to build a URL.:

const url = new URL(this.serviceConfig.url) url.searchParams.set('did', subjectDid) url.searchParams.set('uri', subjectUri)

devinivy · 2024-03-04T15:53:03Z

packages/ozone/src/daemon/blob-diverter.ts

+      method: 'POST',
+      data: imageStream,
+      headers: {
+        Authorization: this.serviceConfig.authToken,


Ideally we wouldn't need to configure the whole authorization header including the auth scheme. Should we be using service auth here, a bearer token, or something else? (CC @bnewbold)

abyss wants HTTP Basic auth, with username admin, and the password as configured "token".

I don't think we have service-auth implemented generically in golang yet, so not super quick to add that (there are other golang clients for this service as well, so need both client+server impls). If we can keep it HTTP Basic that is best from my perspective. I don't know how the axios HTTP library works, maybe we could even encode the password in the URL so there is a single var to configure? abyss is generally not really set up as a regular atproto service, it is most like cardyb and other microservices (one diff is that abyss has pseudo-lexicon endpoints, though the golang side doesn't actually use the lexicon codegen or validation).

devinivy · 2024-03-04T16:02:00Z

packages/ozone/src/daemon/blob-diverter.ts

+      if (!this.serviceConfig.authToken || !this.serviceConfig.url) {
+        throw new Error('Blob divert service not configured')
+      }


If there's no service config, can we avoid creating a BlobDiverter at all? It is a bit awkward to have to check both serviceConfig.authToken and serviceConfig.url in each method and opens the question about how each method should behave if they aren't present.

devinivy · 2024-03-04T16:03:47Z

packages/ozone/src/config/config.ts

+export type BlobReportServiceConfig = {
+  url?: string
+  authToken?: string
+}


It looks like both url and authToken are required in order to use the blob report service. Could we make these both required here, and update OzoneConfig to allow it not to be configured:

export type OzoneConfig = { // ... blobReportService: BlobReportServiceConfig | null }

devinivy · 2024-03-04T16:16:51Z

packages/ozone/src/daemon/blob-diverter.ts

@@ -0,0 +1,229 @@
+import {


Just to toss out an idea— the blob_push_event has an eventType. Could we make blob diversions a new event type and avoid needing to add a new table, new part of the daemon that needs the polling machinery, etc.? I think the blob_push_event is already setup decently well for this situation, may just need to handle the eventType. I could see most of the BlobDiverter class sticking around to encapsulate the specifics about how the blob is pushed out, but not need to handle the polling, etc.

yeah I like that approach better, refactored the code to use that table instead.

devinivy · 2024-03-04T16:22:49Z

packages/ozone/src/mod-service/index.ts

+    if (
+      isModEventDivert(event) &&
+      subjectInfo.subjectUri &&
+      subjectInfo.subjectBlobCids?.length
+    ) {
+      const blobDiverts = await Promise.all(


For other event types we don't take the effect of the event in logEvent(). Instead there's a method that handles it, which gets called within com.atproto.admin.emitModerationEvent after logging the event. For consistency maybe we should move this into that pattern rather than have this outlier.

devinivy · 2024-03-04T16:26:06Z

packages/ozone/src/daemon/blob-diverter.ts

+      const uploadResult = await retryHttp(() =>
+        this.uploadBlob(
+          { imageStream, contentType },
+          { subjectDid, subjectUri },
+        ),
+      )


The imageStream is stateful so it's not going to be super retryable if it gets partially used-up. It's possible that this.getBlob() and this.uploadBlob() should go in the same retry block.

…e-review-state

…roto into divert-blobs

…e-review-state

…blobs

foysalit · 2024-03-07T00:07:29Z

packages/ozone/src/api/admin/emitModerationEvent.ts

 import { subjectFromInput } from '../../mod-service/subject'
 import { ModerationLangService } from '../../mod-service/lang'
 import { retryHttp } from '../../util'
+import { ModeratorOutput, RoleOutput } from '../../auth-verifier'
+
+const handleModerationEvent = async ({


refactored this into a function so that we can execute the entire flow again after divert event.

foysalit · 2024-03-07T00:08:30Z

packages/ozone/src/api/admin/emitModerationEvent.ts

-            content,
-            recipientDid: subject.did,
-          }),
+    if (isModEventDivert(event) && subject.isRecord()) {


sync divert blob sequence here before logging the event.
this way, the moderator will immediately know that the diversion failed and the event won't be logged and won't be takendown.

foysalit · 2024-03-07T00:08:53Z

packages/ozone/src/api/admin/emitModerationEvent.ts

      })

+      // On divert events, we need to automatically take down the blobs
+      if (isModEventDivert(input.body.event)) {


emit takedown event following divert event.

One event expanding into two events on the backend is a little funky. If we find ourselves needing to do this again it might be worth revisiting, seeing if there's a nice way to think about this.

foysalit · 2024-03-08T11:48:13Z

packages/ozone/src/mod-service/index.ts

-        )
-        .returning(['id', 'subjectDid', 'subjectBlobCid', 'eventType'])
-        .execute()
+      const blobEvts = await this.eventPusher.logBlobPushEvent(


This is unrelated to the current implementation but felt nice to keep this refactoring.

foysalit · 2024-03-08T11:48:48Z

packages/ozone/src/mod-service/index.ts

            subjectDid: subject.did,
+            subjectUri: subject.uri || null,


The model seems to expect a subjectUri but it wasn't being passed here.

foysalit added 7 commits February 27, 2024 19:18

🚧 Working through an nullable review state

8c62af0

Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…

c05f2ce

…e-review-state

✅ Update snapshots on some tests

ccc8ab9

✅ Update snapshots on some tests

d7f6538

✅ Add test for reviewOptional status mutation

93ac0e4

🔀 Merge with upstream

1b92941

✨ Add divertBlobs event to send blobs to abyss

ddd9cd5

foysalit requested review from dholms and devinivy February 29, 2024 00:25

foysalit commented Feb 29, 2024

View reviewed changes

foysalit requested a review from bnewbold February 29, 2024 00:40

♻️ Rename reviewOptional -> reviewNone

c5f2c10

foysalit added 5 commits February 29, 2024 13:16

♻️ Rename modEventDivertBlobs -> modEventDivert

4b546e9

🐛 Rename event type checker

64dd7ac

Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…

9851565

…e-review-state

🔀 Merge with upstream

47b5d41

✨ Use pds resolver to get blob straight from pds

071712c

bnewbold approved these changes Feb 29, 2024

View reviewed changes

devinivy reviewed Mar 4, 2024

View reviewed changes

✅ Use FOR UPDATE to respect db transactions

5c0e20a

foysalit added 9 commits March 6, 2024 00:13

♻️ Refactor to use event_pusher table instead of new table

be01cf0

🔀 Merge with upstream

9be9921

Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…

f04c7b6

…e-review-state

Merge branch 'nullable-review-state' of github.com:bluesky-social/atp…

be498d7

…roto into divert-blobs

✨ Bring back missing lines in pnpm-lock

54183d8

🔨 Rebuild?

09f8137

🚨 Formatting

2bd9947

Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…

f52cccd

…e-review-state

🔀 Merge with upstream

2d86f52

Base automatically changed from nullable-review-state to main March 6, 2024 20:30

foysalit added 2 commits March 7, 2024 00:03

♻️ Refactor to divert blob sync

96e2870

Merge branch 'main' of github.com:bluesky-social/atproto into divert-…

104ae95

…blobs

foysalit commented Mar 7, 2024

View reviewed changes

foysalit commented Mar 8, 2024

View reviewed changes

foysalit added 3 commits March 8, 2024 11:53

🧹 Cleanup

93fdbc0

🔀 Merge with upstream

83ce64e

✅ Use modClient seed client in blob-divert test

c7cff52

foysalit mentioned this pull request Mar 8, 2024

✨ Show blob divert event bluesky-social/ozone#33

Merged

devinivy added 4 commits March 11, 2024 16:54

Merge remote-tracking branch 'origin/main' into divert-blobs

641fcd6

update divert blob config to use basic admin auth

f2ac002

fix

0e4c8c8

build

fefd23e

devinivy merged commit 2802880 into main Mar 12, 2024
11 checks passed

devinivy deleted the divert-blobs branch March 12, 2024 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add modEventDivertBlobs event to send blobs to abyss #2238

✨ Add modEventDivertBlobs event to send blobs to abyss #2238

foysalit commented Feb 29, 2024 •

edited

Loading

foysalit Feb 29, 2024

bnewbold Feb 29, 2024

foysalit Feb 29, 2024

devinivy Mar 4, 2024 •

edited

Loading

foysalit Mar 6, 2024

bnewbold commented Feb 29, 2024

foysalit commented Feb 29, 2024

bnewbold left a comment

devinivy Mar 4, 2024

devinivy Mar 4, 2024

devinivy Mar 4, 2024

bnewbold Mar 6, 2024

devinivy Mar 4, 2024

devinivy Mar 4, 2024

devinivy Mar 4, 2024

foysalit Mar 6, 2024

devinivy Mar 4, 2024 •

edited

Loading

devinivy Mar 4, 2024

foysalit Mar 7, 2024

foysalit Mar 7, 2024

foysalit Mar 7, 2024

devinivy Mar 12, 2024

foysalit Mar 8, 2024

foysalit Mar 8, 2024

✨ Add modEventDivertBlobs event to send blobs to abyss #2238

✨ Add modEventDivertBlobs event to send blobs to abyss #2238

Conversation

foysalit commented Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnewbold commented Feb 29, 2024

foysalit commented Feb 29, 2024

bnewbold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foysalit commented Feb 29, 2024 •

edited

Loading

devinivy Mar 4, 2024 •

edited

Loading

devinivy Mar 4, 2024 •

edited

Loading