search

misskey-dev / misskey

🌎 A completely free and open interplanetary microblogging platform 🚀
https://misskey-hub.net/
GNU Affero General Public License v3.0
10.08k stars 1.38k forks source link

参照を解決できないノートがinboxで詰まるらしい #14497

Open KisaragiEffective opened 2 months ago

KisaragiEffective commented 2 months ago

💡 Summary

元々のお題: リプライツリーが長すぎるとinboxで詰まる reported on https://misskey.hinasense.jp/notes/9xsg30de8l4b001m

🥰 Expected Behavior

inboxで詰まらせずに:

  1. チェーン全体の解決を諦めるか
  2. 上限に達した時にそれ以上のチェーンは存在しないものとしてチェーンをちぎる

どうであれinboxには詰まらないようにしたい

🤬 Actual Behavior

inboxで詰まる

📝 Steps to Reproduce

  1. サーバーAでリプライを再帰的にぶら下げる
  2. サーバーAからサーバーBに配送させる
  3. サーバーBのinboxで詰まる

💻 Frontend Environment

* Model and OS of the device(s): ?
* Browser: ?
* Server URL: https://misskey.hinasense.jp/
* Misskey: 2024.8.0-hinasense.jp.5

🛰 Backend Environment (for server admin)

n/a (not server admin)

Do you want to address this bug yourself?

syuilo commented 2 months ago

ちぎる

samunohito commented 2 months ago

ちぎる

acid-chicken commented 2 months ago

image

スレッドチェーンが 100 超えると DoS 判定でアクティビティの受け取りをやめる処理がリトライされるのがそもそも悪そうで リトライ時に一番上まで遡れたところからリトライしてほしさある

mei23 commented 2 months ago

例: Note1→Note2→Note3→Note4→Note5→Note6→Note7→Note8→Note9→Note10 ※ →はリプライの方向 (左が右の投稿にリプライしている)

現状: Note1が来た際にreplyToをNote10まで再帰解決を試みるが、最後まで解決出来ないとNote1~10すべてがエラーになってしまう。 この場合毎回Note1から再試行してしまう。

単純にちぎる案？: 例えばNote7までしか解決出来なかったらそこでNote1~7を確定してコミットしてしまう？ Note1→Note2→Note3→Note4→Note5→Note6→Note7 現状のNoteのDBスキーマ的にはリプライしている or not しかないため、この場合Note7はどこにもリプライしてない状態が受信サーバーでは確定してしまい、追加解決出来る余地もなくリプライが続いていることをユーザーは認知も出来ない。

リトライ時に一番上まで遡れたところからリトライしてほしさある: 実現するには… DBスキーマのNoteのリプライ状態に、あり/なし以外に"このAP IDにリプライをしているが未解決"のステータスを持たせたる必要がある？ また、リプライ解決をNote取得のフォアグラウンドプロセスで行うだけではなく、ジョブキューでのバックグラウンド解決を検討するとベター？

って感じかしら

tamaina commented 2 months ago

ApNoteService.resolveNote~createNoteの引数でcreateNoteの回数をカウントアップして、100を超えたらそこでエラーにして再実行を促すなりキューを(正当な手続きで)登録し直すみたいな感じはどうかしら

↑hit recursion limitの仕組み自体追ってなかったけどresolver.resolveの回数を見てるのか

mei23 commented 2 months ago

ApNoteService.resolveNote~createNoteの引数でcreateNoteの回数をカウントアップして、100を超えたらそこでエラーにして再実行を促すなりキューを(正当な手続きで)登録し直すみたいな感じはどうかしら

それが理想だけど

現状のDBスキーマ的にリプライ先は取得済みでないと存在できないから（つまり再帰エラーはもうリプライが存在しないというステータスにしかできない）

Note.replyId: MiNote['id']
onDelete: 'CASCADE'

それを実現するには、DBスキーマに未解決のAP IDカラムを新設して追加解決を実装する必要があるという感じだわね。

tamaina commented 2 months ago

あっ

tamaina commented 2 months ago

キュー自体に処理中のデータを持たせるみたいなこと考えたけどRedisが肥大化するしなぁ

tamaina commented 2 months ago

replyId/replyをnullにして後で上書きするのが構わなさそうなら、リプライツリーを遡る専用のジョブキューを用意してそういう処理をさせるとか？

ところで、「Inboxが詰まる」事象は100件という数字が大きすぎるという話なのでまだおとなしい数字にする必要がある

mei23 commented 2 months ago
  1. 100もあれば十分だろって考えで、全く解決出来ないよりは100まで解決してその後リプライが続くかは知らんとする (ちぎる)
  2. ＋未解決のリプライAP IDとかをNoteスキーマに保存するようにして、リプライが続いていたらユーザーに教えてあげる (リモートで見てね)
  3. ＋解決しきれないリプライはキュー等を駆使して少しづつでも解決出来るようにする

どこまで頑張るかだわね。

mei23 commented 2 months ago

ちなみにリプライ解決が詰まるパターンって、100到達だけではなく途中のリプライを所有するサーバーが500等の一時的エラーを返すパターンもある気がするのだわ。

tamaina commented 2 months ago

そもそもどうして詰まるというかハングするのかは考えないのかしら

KisaragiEffective commented 2 months ago

(別にこの問題に限らないけど十分丁寧な実装にしたあとのバージョンで何度繰り返しても死ぬものをキューに詰め直すのをやめたい感じ)

KisaragiEffective commented 2 months ago

現状だとリトライする価値のあるエラーとリトライする価値のないエラーが混在してるので運が悪い（？）と詰まる

mei23 commented 2 months ago

ちなみにリプライ解決が詰まるパターンって、100到達だけではなく途中のリプライを所有するサーバーが500等の一時的エラーを返すパターンもある気がするのだわ。

また、リプライ解決の途中で4xx等の恒久的エラーを返された場合は、現状のMisskeyのスキーマ的に存在しないNoteのリプライは存在出来ないので、リプライツリーすべてを存在しないことにして完了してリトライもしない。にするべきだけどちゃんとその仕様で実装されてるかは自信ないのだわ。

mei23 commented 2 months ago

再帰制限100突破でリトライしてしまう → リトライしてはいけない (とりあえずちぎるかもっと頑張るか？) 解決途中で5xx (一時エラー) →とりあえずリトライするしかないか 解決途中で4xx (恒久エラー) →リトライしてはいけない、リプライツリーはすべて存在しないものとして扱わないといけない

みたいな…

KisaragiEffective commented 2 months ago

「リプライチェーンが長すぎるせいで」は関係なくなってきた気がするしissueのタイトル変える？

mei23 commented 2 months ago

確かに、リプライ/引用の解決時エラーの実装ってもやっとしてるわね。

KisaragiEffective commented 2 months ago

とりあえず変えてみた

tamaina commented 2 months ago

リプライツリーの全て(というよりDoS攻撃にはならない範囲)のノートを一回new MiNoteで作っておいて1回のdb transactionで流し込むようにすれば多少ハングはましになるし

リプライツリーすべてを存在しないことに

という処理も可能になりそうだけど、そういう処理(同一トランザクションでidがcascade関係にある)が可能なのかはDB詳しくないのでわからない

あとPollも絡んできて処理は若干複雑になりそう

tamaina commented 2 months ago

「リプライチェーンが長すぎるせいで」は関係なくなってきた気がする

うーん？リプライツリーのノートを大量取得する際にinboxが詰まるというか1つのinboxキューの処理に時間とリソースが食われてる状況だと思うので、「リプライチェーンが長すぎるせいで」は関係なくはないと思うんだけど

KisaragiEffective commented 2 months ago

「リプライチェーンが長すぎるせいで」は関係なくはない

まぁはい もしかしてissue分けたほうがいい？

tamaina commented 2 months ago

分けなくてもいいのでは

あー、色々ごちゃごちゃ言っちゃったけどちぎるって話か（あんまちぎらない前提でごちゃごちゃ喋ってしまった

mei23 commented 2 months ago

あーそういえばキューの最大処理時間で問答無用で終了パターンもあるからめんどくさいわね。やっぱりここまで処理したフラグないとダメかしら。

KisaragiEffective commented 2 months ago

頭の中ではApRenderServiceあたりで何らかの一時中断 (100 recursion, 5xx, timeout) をせざるを得なくなったらジョブキューにサブツリーのrenderを詰め直すで良いんじゃないかなぁとか思ってた 実現できるかどうかは知らない

tamaina commented 2 months ago

うーん、サーバーに必要以上にリプライを持ってても嬉しくないので、ここまで処理したフラグというか、「replyUri」みたいにリプライ先のuriを保存しておいて、UIのボタンでページングのごとく取得できるみたいな感じがサーバー的には嬉しいと思う

実装はやや面倒である

KisaragiEffective commented 2 months ago

後でやるか先にやるかの違いでしか無いかも

KisaragiEffective commented 2 months ago

あまりにたくさんあったら遅延評価されたほうが嬉しい？

KisaragiEffective commented 2 months ago

遅延評価したらrenderが完了するまで照会のloadingがUIで回ることになって困惑しそう

KisaragiEffective commented 2 months ago

まぁユースケース的に大きくなった線形リストみたいなリプライツリーを最後まで追う熱心なユーザーはほとんど居ないか……

tamaina commented 2 months ago

まぁユースケース的に大きくなった線形リストみたいなリプライツリーを最後まで追う熱心なユーザーはほとんど居ないか……

そうこれを期待できる

tamaina commented 2 months ago

そうでなくてもページングで全件取得してくる必要は全くない（10~20件とってきてまだあるようならまたユーザーにボタンを押させれば良い

tamaina commented 2 months ago

UIのボタン

今のUIは「会話を見る」だったわね

KisaragiEffective commented 2 months ago

あれのlimitって深さ方向じゃなくて水平方向だったような…？

mei23 commented 2 months ago

あれのlimitって深さ方向じゃなくて水平方向だったような…？

NoteだろうがUserだろうがもしかしたら固定投稿だろうが区別せずに問答無用で100だったかもだわ

tamaina commented 2 months ago

notes/conversationって深さ方向にしか追わなくない…？

KisaragiEffective commented 2 months ago

notes/conversationって深さ方向にしか追わなくない…？

そうだっけか そうっぽい: https://github.com/misskey-dev/misskey/blob/74c93fcebe1b6fde489470e19808389d13f07a05/packages/backend/src/server/api/endpoints/notes/conversation.ts#L81

tamaina commented 2 months ago

NoteだろうがUserだろうがもしかしたら固定投稿だろうが区別せずに問答無用で100だったかもだわ

AP側のhit recursion limitの100はresolver.resolveを見てるのでノート以外に追加で必要なデータがあったら+1カウントされるわね

tamaina commented 2 months ago

今のUIは「会話を見る」だったわね

これ(notes/conversationを呼び出す処理)、NoteDetailedにしかないし10件取得したらそれで満足やろみたいな仕様になってるわね

anatawa12 commented 2 months ago

若干オフトピ: 正直足りないことちょくちょくあるので見るベージはほしい

tamaina commented 2 months ago

replyUriを保存した場合はNoteEntityService.pack (detail: true)の呼び出しによってreplyUriを解決するみたいな感じでいいのか？(inboxキュー処理内でリプライ先を解決する必要すらない?)

いやよくない、祖先が削除されていたらその子供はCASCADE削除されなければならない

オフトピ:

conversationの話ね

tamaina commented 2 months ago

祖先が削除されていたらその子供はCASCADE削除されなければならない

（そもそもこの仕様自体顰蹙買ってるわけだけど: Related to https://github.com/misskey-dev/misskey/issues/7342

tamaina commented 2 months ago

そしてこの仕様がある限り、データベースに追加したりストリーム配信する前に全ての親を遡って取得して削除されていないか確認する必要がある、ちぎってもいけない

KisaragiEffective commented 2 months ago

そうなの？子孫をちぎるのは問題ないと思うのだけど

tamaina commented 2 months ago

今子孫関係なくない？というかinboxでやってきたおnewのノートに子孫がいるわけない

KisaragiEffective commented 2 months ago

そっか