Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix/chore] oauth entropy fix + media cleanup tasks rewrite #1853

Conversation

NyaaaWhatsUpDoc
Copy link
Member

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc commented Jun 1, 2023

Description

  • creates new cleaner package for filesystem / database cleaning / fixing tasks
  • moves media cleaning to cleaner package, mostly rewritten in the process
  • combines multiple of the pruning jobs as the logic for local + remote is (near enough) the same
  • adds media cleanup task to ensure media entry cache states are in sync with storage
  • fixes entropy generation for new "router sessions" (oauth registration flow state key generation)
  • much more thorough removal on delete of media attachments from database

Still needed

- finish tests fixing move media cleaning tests (combine local + remote prunes)
- figure out the testsuite emoji files having a fixed instance ID unrelated to test suite generated one
- test function for the new media FixCacheStates() cleaner function
- migration script to regenerate "router session" entropy
- enable passing in activitypub raw object to enrichAccount again (? this may be source of the missing profile image bug)
- figure out actual cause of the missing profile pictures bug (maybe old media cleanup related)
- move the emoji refetch code over to the cleaner package also
- cleanup the db.Media{} GetAttachments___() code, the changes here haven't be fully code commented yet and there's probably some overlapping / now unused functions that can be removed

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

@NyaaaWhatsUpDoc
Copy link
Member Author

This is the code I was discussing in matrix @tsmethurst, both the general little entropy fix and at least a solution for the missing profile images bug, even if not necessarily addressing the root cause. If the root cause was the old media cleanup code then it might actually be fixed, if not then the code here will at least be a mitigation for it.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/media-cleaning-rewrite branch from d83f8b6 to a3eab8a Compare June 4, 2023 11:59
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/media-cleaning-rewrite branch 3 times, most recently from daa97f5 to 3c83670 Compare June 17, 2023 14:39
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as ready for review June 17, 2023 19:29
@NyaaaWhatsUpDoc
Copy link
Member Author

this is ready now @tsmethurst

the other remaining changes that i crossed out above (though not exactly) i'll make into a second PR

@@ -214,7 +211,7 @@ func (m *Manager) ProcessMedia(ctx context.Context, data DataFunc, accountID str
func (m *Manager) PreProcessEmoji(ctx context.Context, data DataFunc, shortcode string, emojiID string, uri string, ai *AdditionalEmojiInfo, refresh bool) (*ProcessingEmoji, error) {
instanceAccount, err := m.state.DB.GetInstanceAccount(ctx, "")
if err != nil {
return nil, fmt.Errorf("preProcessEmoji: error fetching this instance account from the db: %s", err)
return nil, gtserror.Newf("error fetching this instance account from the db: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to use %w for these errors so we can unwrap them later if need be, right? Or was this purposeful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes i just made by doing a find and replace on existing error statements, otherwise i would have updated them to use %w

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent changes!!

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
…investigate)

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
…rd??)

Signed-off-by: kim <grufwub@gmail.com>
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/media-cleaning-rewrite branch from 0a464c8 to 0a395bd Compare June 22, 2023 18:29
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit 9a22102 into superseriousbusiness:main Jun 22, 2023
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc deleted the chore/media-cleaning-rewrite branch June 22, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants