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

リモートユーザーの一時停止を解除した後は user.isDeleted = true #13504

Open
1 task done
Hoto-Cocoa opened this issue Mar 3, 2024 · 17 comments · May be fixed by #13890
Open
1 task done

リモートユーザーの一時停止を解除した後は user.isDeleted = true #13504

Hoto-Cocoa opened this issue Mar 3, 2024 · 17 comments · May be fixed by #13890
Labels
⚠️bug? This might be a bug

Comments

@Hoto-Cocoa
Copy link
Contributor

Hoto-Cocoa commented Mar 3, 2024

💡 Summary

この問題がuser suspendのために発生した問題であるかどうかは不明ですが、推定される唯一の理由です。

私はAインスタンスで活動していますが、Bインスタンスに私とのリアクション、フォロー活動が全部連合されず、Bインスタンスの主人と協力して問題を解決していました。

ロギングで確認した結果、 getAuthUserFromApIdがnullを返し、これを確認した結果isDeletedがtrueの場合にのみnullを返すことを確認してSQL Queryを実行してみると、実際にisDeletedがtrueであり、これをfalseに変換した後 再起動すると問題が解決しました。

以前にスパムのメンション事態の際、私のアカウントが誤って停止する事態がありました。 これがisDeletedがtrueである唯一の理由ではないかと推測します。

Aインスタンスの私のアカウントはisRootがtrueなので、Aインスタンスではaccount delete activityはなかったと思います。 当然、Aインスタンスでアカウントを停止したりすることはなく、Bインスタンスで私のアカウントを一時的に停止したことが推測される唯一の理由です。

停止解除後に#13256の問題と同じことがあり、取り扱う内容もある程度似ていますが、同じ問題ではないようで、別の問題として公開します。

🥰 Expected Behavior

isDeleted = false

🤬 Actual Behavior

isDeleted = true

📝 Steps to Reproduce

  1. 2 つのインスタンスを作成します。
  2. 最初のインスタンスのユーザーを 2 番目に停止してから解放します。
  3. 問題が発生すると推定されます。

💻 Frontend Environment

* Model and OS of the device(s): Windows 10 21H2
* Browser: Chrome 122.0.6261.95
* Server URL: https://hoto.moe
* Misskey: 2024.3.1

🛰 Backend Environment (for server admin)

* Installation Method or Hosting Service: Docker
* Misskey: 2024.3.1
* Node: (stock docker image)
* PostgreSQL: 15
* Redis: 7
* OS and Architecture: Ubuntu 22.04.2

Do you want to address this bug yourself?

  • Yes, I will patch the bug myself and send a pull request
@Hoto-Cocoa Hoto-Cocoa added the ⚠️bug? This might be a bug label Mar 3, 2024
@Hoto-Cocoa
Copy link
Contributor Author

isDeleted だけ true で、実際に削除動作が進められていませんでした。
今考えてみると停止が解けた直後には連合がうまくいったようですが…理由がわかりません。

@Hoto-Cocoa
Copy link
Contributor Author

Now I have faced a new case that may be related to this issue:

What did

  • Instance P suspended local user P1.
  • Instances H, K, B, N, and others that federated to Instance P received ActivityPub Delete Activity as following code:
    const content = this.apRendererService.addContext(this.apRendererService.renderDelete(this.userEntityService.genLocalUserUri(user.id), user));
  • Instance P unsuspended local user P1.
  • Instances H, K, B, N, and others that federated to Instance P received ActivityPub Undo Delete Activity as following code:
    const content = this.apRendererService.addContext(this.apRendererService.renderUndo(this.apRendererService.renderDelete(this.userEntityService.genLocalUserUri(user.id), user), user));

As a note, User P1 doesn't follow any user in local or remote, both at suspended/unsuspended time.

What happened

  • User P1 tried to follow the remote user in Instance H, K, B, N but not works; It just shows "Processing" forever.
  • In Instance H and N, P1 flagged isDeleted as true, but K is false and B doesn't have a record. (after getting the user manually from the instance P, that flag is false as the same as K)
    • Instances H and N are not federating normally to user P1, as expected.
    • Instances K and B are not federating normally as same as H and N even they are not flagged isDeleted as true.

It seems any Misskey-based instances are not federating normally user P1 except Instance F(Firefish-based) and Misskey.IO, As user P1 can remotely follow the user in those instances.

So now: I think ActivityPub Undo Delete Activity is not processed correctly (H, N), and something makes Misskey assume User P1 isDeleted as true (K, B).

After Instance H updates User P1 isDeleted to false, the federation works normally as in the first case.

I will test this on Intranet by creating some testing-purpose instances.

@Hoto-Cocoa
Copy link
Contributor Author

I did some research and It may:

  • If database row cleared, isDeleted will be false after querying but CacheService.userByIdCache(MemoryKVCache) seems not cleared
  • In many cases, "isDeleted" is not true in database but cache does
  • I think "isDeleted" will be true in database when: User unsuspended before misskey account deletion is done(For example, data is too many to purge fastly or queue delayed), or job is failed

So...

  • I think MemoryKVCache row should removed after account deletion is done
  • I think "isDeleted" should be false after account deletion is aborted
  • Also, I think ApInboxService.deleteActor should call createDeleteAccountJob with { soft: true } argument like how /api/admin/accounts/delete endpoint works.

I will open PR after do some researches more.

@Hoto-Cocoa
Copy link
Contributor Author

Also, I think ApInboxService.deleteActor should call createDeleteAccountJob with { soft: true } argument like how /api/admin/accounts/delete endpoint works.

However, Misskey can't determine "Why actor deleted from remote?" so processing all remote actor deletion as soft deletion may not good. If to do this, run hard actor deletion job after specified time may required.

@Hoto-Cocoa
Copy link
Contributor Author

Currently, Undo Delete Actor activity is not processed.

  • If cache remained, authUser will be null, then not processing activity.
    if (authUser == null) {
    throw new Bull.UnrecoverableError('skip: failed to resolve user');
    }
  • If authUser is not null, ApInboxService.undo doesn't have Undo Delete activity processing logic, then It will be ignored.
    private async undo(actor: MiRemoteUser, activity: IUndo): Promise<string> {
    if (actor.uri !== activity.actor) {
    throw new Error('invalid actor');
    }
    const uri = activity.id ?? activity;
    this.logger.info(`Undo: ${uri}`);
    const resolver = this.apResolverService.createResolver();
    const object = await resolver.resolve(activity.object).catch(e => {
    this.logger.error(`Resolution failed: ${e}`);
    throw e;
    });
    // don't queue because the sender may attempt again when timeout
    if (isFollow(object)) return await this.undoFollow(actor, object);
    if (isBlock(object)) return await this.undoBlock(actor, object);
    if (isLike(object)) return await this.undoLike(actor, object);
    if (isAnnounce(object)) return await this.undoAnnounce(actor, object);
    if (isAccept(object)) return await this.undoAccept(actor, object);
    return `skip: unknown object type ${getApType(object)}`;
    }

So one of my think might wrong:

I think "isDeleted" will be true in database when: User unsuspended before misskey account deletion is done(For example, data is too many to purge fastly or queue delayed), or job is failed

It happens only at job failed time or something else.

@Hoto-Cocoa
Copy link
Contributor Author

/api/admin/accounts/delete is not used, but /api/admin/delete-account does.

The API currently used is not using soft delete, so I think I need to just use hard delete.

I'm trying to re-run delete account job when remote actor is in local database and isDeleted is true but job is not running.

@Hoto-Cocoa Hoto-Cocoa linked a pull request May 27, 2024 that will close this issue
5 tasks
@Hoto-Cocoa
Copy link
Contributor Author

Hoto-Cocoa commented May 27, 2024

The PR #13890 will fixes recent case but not first case that is:

  • Instance K suspended remote user H1 who in Instance H, but not deleted that user
  • However, User H1 was isDeleted = true after unsuspended in Instance K
  • Instance H never suspended(or trying to delete) user H1, because the user is root

So now I can't figure how this caused but the PR fixes recent case perfectly at least, and I don't think so that I will find why first case caused. (I'm not operator of Instance K and I think logs are deleted in past)

@atsu1125
Copy link
Contributor

atsu1125 commented Jun 1, 2024

Misskey needs to remain Suspended State of Remote account, so account deletion job for remote Suspended User does not delete him completely from DB.
Therefore, isDeleted state of Remote Suspended User should turn to false when the user get unsuspended. (currently not)

@Hoto-Cocoa
Copy link
Contributor Author

@atsu1125 If Misskey Instance A suspends User A1 and Instance A federated to Misskey Instance B, Misskey sends ActivityPub Activity contains Actor Delete, which same as User Delete. So Remote Misskey Instance(In this case Instance B) should just remove account. And If Remote Misskey Instance unsuspends User(In this case User A1) and Misskey Instance has that user in record with isDeleted = true, All Activity completed ignored so user cannot recovered.

If you are saying about A1 suspended in B, In second case User P1 not suspended at all remote instances but federation broken. I think this is bug. But I didn't tested A1 suspended in B and A1 also suspended in A so Delete Actor activity sent, but I think that's not make sense since Misskey completely ignores all activity including Delete Actor If actor user suspended in Local.

@Hoto-Cocoa
Copy link
Contributor Author

So, this is what I found:

  • Misskey sends activity as same as deleting account when local user suspended
  • Misskey processes that activity, but If actor user is suspended in Local then activity is ignored so user will not deleted; otherwise Misskey make job for delete that remote user
  • If that job failed, user remains in database but isDeleted = true <- Problem 1
  • If job success then Misskey remove that user from database but cache remains <- Problem 2

So, this is what I think:

  • Misskey can't remain remote user's remote suspended state because there is no activity payload for that at this time (Misskey can't know remote user is suspended in remote instance at all time as Misskey just not respond about suspended user)
  • If remote instance suspends their user that suspended in local then sends delete actor activity but Misskey ignores locally suspended user so that activity will not processed (This is same when even that user really removed in remote)

So I think that would be no cause for concern.

@atsu1125
Copy link
Contributor

atsu1125 commented Jun 1, 2024

Misskey doesn't delete User A1 from User Table on DB when Instance B deletes User A1 as Moderation. Because User A1 is still active on Instance A, so if User A1 is deleted from User Table on DB of Instance B, User A1 is newly created on Instance B even A1 is previously deleted on Instance B.
If User A1 is Suspended on Instance B, and admin of Instance B wants to delete Note and drive of A1, he might try to delete User A1, but Suspended State of A1 will be deleted if A1 is completely deleted on Instance B.

@Hoto-Cocoa
Copy link
Contributor Author

Now I saying about not: suspending remote user in local
Now I saying about: suspending remote user in remote (and unsuspending remote user in remote)

For example, suspending @[email protected] in ins1.mk and unsuspending.

@Hoto-Cocoa
Copy link
Contributor Author

Hoto-Cocoa commented Jun 1, 2024

And,

Misskey doesn't delete User A1 from User Table on DB when Instance B deletes User A1 as Moderation.

This is false in Misskey 2024.3.1 version. If Instance A moderator deletes remote user B1 and when Instance A needs fetch(note federated, user queried, and other reason) user B1, Misskey just fetches User B1 as normal new user even User B1 was suspended before delete as Misskey always completely delete user in database If requested in Control Panel.

@atsu1125
Copy link
Contributor

atsu1125 commented Jun 1, 2024

I understand.
And yes.

  1. Misskey should not send Delete Activity when moderator suspend Local User, because the user is not actually deleted and follow relationships are not get synced with Remote Followers.
  2. Misskey should receive Delete Activity of Remote Suspended User and proceed Account Deletion Job for him.

@Hoto-Cocoa
Copy link
Contributor Author

Misskey should not send Delete Activity when moderator suspend Local User, because the user is not actually deleted and follow relationships are not get synced with Remote Followers.

Yes, I think so. But ActivityPub standard is not has activity type for this so I think Misskey don't have other options that fits to all ActivityPub implementations.

@atsu1125
Copy link
Contributor

atsu1125 commented Jun 1, 2024

I have looked api/admin/accounts/delete but currently api/admin/delete-account is used so I understand that misskey trys to delete Remote account completely from DB.

soft: true, // リモートユーザーの削除は、完全にDBから物理削除してしまうと再度連合してきてアカウントが復活する可能性があるため、soft指定する

@Hoto-Cocoa
Copy link
Contributor Author

I have looked api/admin/accounts/delete but currently api/admin/delete-account is used so I understand that misskey trys to delete Remote account completely from DB.

soft: true, // リモートユーザーの削除は、完全にDBから物理削除してしまうと再度連合してきてアカウントが復活する可能性があるため、soft指定する

Thanks for lookup reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️bug? This might be a bug
Projects
Development

Successfully merging a pull request may close this issue.

2 participants