firehose tombstones #1017

jesopo · 2023-05-09T23:10:01Z

to be honest this change seems so simple that i assume the reason it wasn't implemented yet is less to do with spare time and more to do with technical details that need to be worked out, but i thought i'd try my luck anyway

jesopo · 2023-05-10T01:51:54Z

(test on its way...)

packages/pds/tests/sync/subscribe-repos.test.ts

packages/pds/src/sequencer/events.ts

dholms · 2023-05-10T15:58:49Z

it actually totally had to do with time 😅

we just have a million small things to do that would only take 20 min a piece, but haven't gotten around to

Besides getting the invalidation logic in here, it looks good!

thanks for doing this!

jesopo · 2023-05-10T20:44:04Z

packages/pds/src/sequencer/events.ts

@@ -151,6 +167,10 @@ export const invalidatePrevHandleOps = async (db: Database, did: string) => {
  return invalidatePrevSeqEvts(db, did, ['handle'])
 }

+export const invalidatePrevOps = async (db: Database, did: string) => {
+  return invalidatePrevSeqEvts(db, did, ['append', 'handle', 'rebase'])


enumerating all the different event types here feels sort of bad and prone to breaking next time an event type is added. i guess i could change the db query in invalidatePrevSeqEvts to just not have a where eventType = clause for this case? thoughts?

Oh yknow what I just realized?

We actually delete all the repo_seq events for a user when they delete their account, so there won't be any events to invalidate 😅

I think you're actually good the way it was. Sorry about that!

Just to prevent a potential footgun where we clear out all of an account's repo_seqs after we create the tombstone op, maybe we add a where eventType != 'tombstone on the repo_seqs deletion in the repo service

jesopo · 2023-05-10T20:45:02Z

packages/pds/tests/sync/subscribe-repos.test.ts

+        if (
+          (frame.header.t === "#commit" && (frame.body as CommitEvt).repo === userDid)
+          || (frame.header.t === "#handle" && (frame.body as HandleEvt).did === userDid)
+          || (frame.header.t === "#tombstone" && (frame.body as TombstoneEvt).did === userDid)


same thing here wrt enumerating all the types

though in this situation i don't think there's a better way to extract a did from body without doing this

Broken60 · 2023-05-14T13:24:28Z

it actually totally had to do with time 😅

we just have a million small things to do that would only take 20 min a piece, but haven't gotten around to

Besides getting the invalidation logic in here, it looks good!

thanks for doing this!

H

dholms · 2023-05-26T00:23:21Z

hey @jesopo wondering if you're planning on getting back to this or if i should get it over the line?

jesopo · 2023-05-26T07:56:05Z

what's missing

dholms · 2023-05-26T15:51:17Z

The comments I left here: #1017 (comment)

I led you astray earlier & we actually don't need to invalidate events on the stream, we fully delete them on account deletion. Sorry about the confusion on this!

Plus, Adding in the extra check on account deletion to ensure that we don't delete the tombstone event

jesopo · 2023-05-26T17:46:44Z

ah yeah sorry missed that. I'll look at it shortly

jesopo · 2023-05-28T01:03:03Z

@dholms how's that?

dholms

yup this looks good! thanks for putting this together

running tests then i'll merge & deploy soon

dholms · 2023-05-10T16:00:42Z

packages/pds/tests/sync/subscribe-repos.test.ts

+
+    const tombstoneEvts = getTombstoneEvts(evts.slice(-2))
+    verifyTombstoneEvent(tombstoneEvts[0], baddie1)
+    verifyTombstoneEvent(tombstoneEvts[1], baddie2)


after adding in invalidation - an extra check in this test to ensure that it properly invalidates previous handle & commit events would be cool 👌

dholms · 2023-05-11T01:13:44Z

packages/pds/src/sequencer/events.ts

@@ -151,6 +167,10 @@ export const invalidatePrevHandleOps = async (db: Database, did: string) => {
  return invalidatePrevSeqEvts(db, did, ['handle'])
 }

+export const invalidatePrevOps = async (db: Database, did: string) => {
+  return invalidatePrevSeqEvts(db, did, ['append', 'handle', 'rebase'])


Oh yknow what I just realized?

We actually delete all the repo_seq events for a user when they delete their account, so there won't be any events to invalidate 😅

I think you're actually good the way it was. Sorry about that!

dholms · 2023-05-11T01:18:36Z

packages/pds/src/sequencer/events.ts

@@ -151,6 +167,10 @@ export const invalidatePrevHandleOps = async (db: Database, did: string) => {
  return invalidatePrevSeqEvts(db, did, ['handle'])
 }

+export const invalidatePrevOps = async (db: Database, did: string) => {
+  return invalidatePrevSeqEvts(db, did, ['append', 'handle', 'rebase'])


Just to prevent a potential footgun where we clear out all of an account's repo_seqs after we create the tombstone op, maybe we add a where eventType != 'tombstone on the repo_seqs deletion in the repo service

jesopo · 2023-05-30T18:24:20Z

oh i see, your last round of comments were not actually submitted yet and now they are. i shall take a look

jesopo · 2023-05-30T18:26:19Z

(also, i will fix tests)

jesopo · 2023-05-30T18:45:50Z

Oh yknow what I just realized?

We actually delete all the repo_seq events for a user when they delete their account, so there won't be any events to invalidate 😅

I think you're actually good the way it was. Sorry about that!

are you sure about this? packages/pds/tests/sync/subscribe-repos.test.ts does not return invalidated events is returning a handle event for a user deleted in the previous test (account deletions invalidate all seq ops)

jesopo · 2023-05-30T18:59:53Z

atproto/packages/pds/src/services/account/index.ts

Lines 400 to 419 in 95252dd

    
           async deleteAccount(did: string): Promise<void> { 
        
             // Not done in transaction because it would be too long, prone to contention. 
        
             // Also, this can safely be run multiple times if it fails. 
        
             await this.db.db 
        
               .deleteFrom('refresh_token') 
        
               .where('did', '=', did) 
        
               .execute() 
        
             await this.db.db 
        
               .deleteFrom('user_account') 
        
               .where('user_account.did', '=', did) 
        
               .execute() 
        
             await this.db.db 
        
               .deleteFrom('user_state') 
        
               .where('user_state.did', '=', did) 
        
               .execute() 
        
             await this.db.db 
        
               .deleteFrom('did_handle') 
        
               .where('did_handle.did', '=', did) 
        
               .execute() 
        
           }

I don't see repo_seq in here

dholms · 2023-05-30T20:51:28Z

yeah that delete happens in the repo service

services/repo/index.ts line 341
await this.db.db.deleteFrom('repo_seq').where('did', '=', did).execute()

i'll have to check this out to see where the hiccup is happening 🤔

jesopo · 2023-05-30T23:07:15Z

race you 🏃

jesopo · 2023-05-31T16:46:03Z

figured it out - my test was only calling account delete, not repo delete

dholms · 2023-05-31T19:51:45Z

(I think CI should start running automatically for your PRs after this one lands)

but in the mean time, mind running yarn verify to fix linting issues?

dholms · 2023-05-31T19:51:55Z

& then we'll finally get this in!

jesopo · 2023-06-02T17:49:00Z

done(?)

jesopo · 2023-06-02T21:18:46Z

i believe this won't send tombstones for account takedowns. i imagine that's desired behaviour anyway, but just wanted to check

dholms · 2023-06-08T16:44:30Z

(don't hate me there's a merge conflict)

I actually tried to resolve this myself & some of the tests are failing 🤔

I'm really not sure why as none of your changes affect sequencing logic, need to look into it more...

Similar failures as over here: #1173
appears to be double-sequencing some events

jesopo · 2023-06-08T17:27:51Z

I'll check this eve :)

dholms

* firehose tombstones * return tombstones from sequencer requestSeqRange * firehose tombstone test * oops semicolons :3 * yes this can be const * fix verifyTombstoneEvent func signature * invalidate all sequence events upon tombstone * don't manually purge all seqs, test for tombstone after delete * actually fully delete account * fix linting --------- Co-authored-by: Devin Ivy <devinivy@gmail.com>

ericvolp12 reviewed May 10, 2023

View reviewed changes

packages/pds/tests/sync/subscribe-repos.test.ts Outdated Show resolved Hide resolved

dholms reviewed May 10, 2023

View reviewed changes

packages/pds/src/sequencer/events.ts Show resolved Hide resolved

jesopo force-pushed the firehose-tombstone branch from f207bce to 5a21854 Compare May 10, 2023 15:59

jesopo commented May 10, 2023

View reviewed changes

jesopo requested a review from dholms May 10, 2023 20:44

jesopo commented May 10, 2023

View reviewed changes

jesopo closed this May 26, 2023

jesopo reopened this May 26, 2023

jesopo force-pushed the firehose-tombstone branch from 08b9769 to 06f240c Compare May 28, 2023 01:02

dholms approved these changes May 30, 2023

View reviewed changes

jesopo added 8 commits May 30, 2023 18:58

firehose tombstones

80c20b2

return tombstones from sequencer requestSeqRange

1ffc2b3

firehose tombstone test

b56b086

oops semicolons :3

509ce52

yes this can be const

fdbe500

fix verifyTombstoneEvent func signature

416fbce

invalidate all sequence events upon tombstone

d425eab

don't manually purge all seqs, test for tombstone after delete

3dff7d4

actually fully delete account

7874d72

jesopo force-pushed the firehose-tombstone branch from 06f240c to 7874d72 Compare May 31, 2023 16:45

jesopo requested a review from dholms May 31, 2023 16:46

fix linting

a50e585

dholms changed the base branch from main to firehose-tombstones June 7, 2023 15:49

dholms changed the base branch from firehose-tombstones to main June 7, 2023 16:00

Merge remote-tracking branch 'origin/main' into firehose-tombstone

bf4adeb

dholms approved these changes Jun 15, 2023

View reviewed changes

dholms merged commit d9ac72e into bluesky-social:main Jun 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

firehose tombstones #1017

firehose tombstones #1017

jesopo commented May 9, 2023

jesopo commented May 10, 2023

dholms commented May 10, 2023 •

edited

Loading

jesopo May 10, 2023

dholms May 11, 2023

dholms May 11, 2023

jesopo May 10, 2023

jesopo May 10, 2023

Broken60 commented May 14, 2023

dholms commented May 26, 2023

jesopo commented May 26, 2023

dholms commented May 26, 2023

jesopo commented May 26, 2023

jesopo commented May 28, 2023

dholms left a comment

dholms May 10, 2023

dholms May 11, 2023

dholms May 11, 2023

jesopo commented May 30, 2023

jesopo commented May 30, 2023

jesopo commented May 30, 2023

jesopo commented May 30, 2023

dholms commented May 30, 2023

jesopo commented May 30, 2023

jesopo commented May 31, 2023

dholms commented May 31, 2023

dholms commented May 31, 2023

jesopo commented Jun 2, 2023

jesopo commented Jun 2, 2023

dholms commented Jun 8, 2023 •

edited

Loading

jesopo commented Jun 8, 2023

dholms left a comment

firehose tombstones #1017

firehose tombstones #1017

Conversation

jesopo commented May 9, 2023

jesopo commented May 10, 2023

dholms commented May 10, 2023 • 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

Broken60 commented May 14, 2023

dholms commented May 26, 2023

jesopo commented May 26, 2023

dholms commented May 26, 2023

jesopo commented May 26, 2023

jesopo commented May 28, 2023

dholms 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

jesopo commented May 30, 2023

jesopo commented May 30, 2023

jesopo commented May 30, 2023

jesopo commented May 30, 2023

dholms commented May 30, 2023

jesopo commented May 30, 2023

jesopo commented May 31, 2023

dholms commented May 31, 2023

dholms commented May 31, 2023

jesopo commented Jun 2, 2023

jesopo commented Jun 2, 2023

dholms commented Jun 8, 2023 • edited Loading

jesopo commented Jun 8, 2023

dholms left a comment

Choose a reason for hiding this comment

dholms commented May 10, 2023 •

edited

Loading

dholms commented Jun 8, 2023 •

edited

Loading