Skip to content

Q&Aを自動的にクローズする機能のバグ修正 #9024

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.

Sign up for GitHub

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

Merged
komagata merged 4 commits into from
Aug 18, 2025
Merged

Q&Aを自動的にクローズする機能のバグ修正 #9024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12880da
一定期間コメントのないQ&Aを閉じる定期処理が実行されないバグを修正
ryufuta Aug 9, 2025
e1d82d4
Q&A自動クローズ機能の動作確認用の初期データを追加
ryufuta Aug 10, 2025
0c33a14
Q&A自動クローズの処理中にユーザーがベストアンサーを選択するとその回答が削除されるバグを修正
ryufuta Aug 10, 2025
4b5763a
初期データの文言を定数参照に変更
ryufuta Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions app/controllers/scheduler/daily/auto_close_questions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
# frozen_string_literal: true

module Scheduler
module Daily
class AutoCloseQuestionsController < ApplicationController
def show
QuestionAutoCloser.post_warning
QuestionAutoCloser.close_and_select_best_answer
head :ok
end
end
class Scheduler::Daily::AutoCloseQuestionsController < SchedulerController
def show
QuestionAutoCloser.post_warning
QuestionAutoCloser.close_and_select_best_answer
Comment on lines +5 to +6
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

QuestionAutoCloser内部のN+1/並行実行による重複発生の可能性を軽減できるか確認を。

呼び出し先では Question.not_solved.find_each + question.answers.select/any? でRuby側フィルタを行っており、大量件数でN+1やメモリ負荷になり得ます。また同時起動時に警告コメントの重複生成が理論上は起こり得ます（判定→作成が非原子的）。性能・整合性要件次第ですが、将来的に以下の改善余地があります。

  • 予告/クローズ対象の絞り込みをSQL側に寄せる（LIKE/正規表現・サブクエリ・EXISTS活用）＋ includes(:answers) 等で事前ロード。
  • 1質問あたり1警告に絞るためのDB制約や with_lock の活用、もしくは何らかの冪等キーでのUpsert。

リポジトリ全体での該当箇所を洗い出す補助スクリプトです（実際のリファクタは別PRで十分です）:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "[scan] QuestionAutoCloser の候補ボトルネック（answers.select/any? など）"
fd -a question_auto_closer.rb | xargs -I{} rg -n -C2 -e 'find_each' -e '\.answers\.select' -e '\.answers\.any\?' "{}" || true

Length of output: 974

要対応: QuestionAutoCloser の N+1 と同時実行による重複生成リスク（確認済み））

リポジトリ内検索の結果、QuestionAutoCloser 内で find_each と Ruby 側フィルタ（answers.select / answers.any?）が確認されました。大量データで N+1 やメモリ負荷、かつ「判定 → 作成」が非原子的なため同時実行時に警告コメントが重複して生成されるリスクがあります。

該当箇所（確認済み）

  • app/controllers/scheduler/daily/auto_close_questions_controller.rb — lines 5–6: QuestionAutoCloser.post_warning / QuestionAutoCloser.close_and_select_best_answer
  • question_auto_closer.rb — find_each のループ（lines 16, 27）
  • question_auto_closer.rb — question.answers.select（line 60）
  • question_auto_closer.rb — question.answers.any?（line 67）

推奨対応（優先順）

  • 判定ロジックを可能な限りSQL側へ移行する（answers.where(...).exists? / joins やサブクエリ／EXISTS を利用）。LIKE/正規表現はDB依存なので注意。
  • N+1対策として includes(:answers) や必要カラムのみを事前ロードする（または join + distinct で絞る）。
  • 重複防止は DB レベルのユニーク制約 ＋ create_or_find_by / upsert、あるいは with_lock／トランザクション／アドバイザリロックで原子的に処理する。
  • バッチ処理・メモリ対策：find_each のバッチサイズ調整や不要オブジェクト生成の抑制。

具体的なリファクタは別PRで問題ありませんが、上記を踏まえて優先的に対応してください。

Copy link
Copy Markdown
Contributor Author

@ryufuta ryufuta Jan 8, 2026

Choose a reason for hiding this comment

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

@coderabbitai

大量データで N+1 やメモリ負荷

過去の運用実績からすると、未解決の質問（Question.not_solved）は5件以下、一つの質問に対する回答は平均数件で多くても数十件なので、メモリ負荷はあまり意識しなくて大丈夫です。
ただし、指摘の通り現状のコードは非効率なのでN+1対策とロジックのSQL側への移行は別PRで検討します。

「判定 → 作成」が非原子的なため同時実行時に警告コメントが重複して生成されるリスクがあります

app/controllers/scheduler/daily/auto_close_questions_controller.rbのメソッドはdailyで自動的に一度だけ呼び出されるため同時実行リスクはありません。

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

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

@ryufuta, 運用実績に基づく詳細な情報をありがとうございます。

了解しました:

  • 未解決質問が5件以下、回答も数件〜数十件程度という実績であれば、現状の実装でメモリ負荷は問題ないですね
  • N+1対策とSQL側への移行は別PRで検討いただけるとのこと、承知しました
  • dailyで1回のみ自動実行される仕様であれば、同時実行による重複リスクは確かに存在しませんね

本PRはバグ修正が目的ですので、パフォーマンス最適化は別PRで対応するアプローチが適切だと思います。貴重なコンテキストをご共有いただき、ありがとうございました！

🧠 Learnings used 
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9323
File: app/models/question_auto_closer.rb:45-51
Timestamp: 2025-12-03T06:39:24.048Z
Learning: fjordllc/bootcamp の QuestionAutoCloser において、create_warning_message で発行される 'answer.create' イベントによってAnswererWatcherがシステムユーザーを質問の監視者として追加する動作は、プロダクトオーナーの判断により問題ないとされており、現状のまま維持される。

Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9323
File: app/models/question_auto_closer.rb:45-51
Timestamp: 2025-11-25T05:42:10.478Z
Learning: fjordllc/bootcamp の QuestionAutoCloser において、create_warning_message で発行される 'answer.create' イベントは、システムユーザーによる自動クローズ警告メッセージを質問者と既存の監視者に通知するための意図的な設計である。AnswerNotifier と NotifierToWatchingUser による通知は期待される動作。

Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 9202
File: config/initializers/active_support_notifications.rb:6-7
Timestamp: 2025-11-03T04:53:33.424Z
Learning: app/controllers/api/answers_controller.rb の destroy アクションにおいて、answer.destroy イベントは `if answer.type == 'CorrectAnswer'` 条件付きで発行される。通常の Answer 削除では未解決問題数に影響しないため、ベストアンサー (CorrectAnswer) 削除時のみ AnswerCacheDestroyer によるキャッシュクリアが必要。

Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。

Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。

Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。

Learnt from: e-yanagita-gs
Repo: fjordllc/bootcamp PR: 8771
File: app/controllers/welcome_controller.rb:57-57
Timestamp: 2025-06-30T01:07:46.192Z
Learning: プロジェクトでは、仕様上データの存在が保証されている場合、nil処理を省略することがある。特にFAQ表示のような特定機能に特化したPRでは、データの存在を前提とした実装が採用される。

Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.036Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。

Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: db/fixtures/buzzes.yml:0-0
Timestamp: 2025-09-04T01:27:23.422Z
Learning: The buzz_controller.rb was indeed deleted, but there are still two remaining items from the old buzz single page implementation that need cleanup: the data migration file db/data/20230726024528_create_buzz.rb and the old buzz route in config/routes.rb line 136. These should be updated/removed to complete the buzz page redesign migration.

Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: db/fixtures/buzzes.yml:0-0
Timestamp: 2025-09-04T01:27:23.422Z
Learning: The buzz_controller.rb and related data migration files were deleted as part of the Buzz page redesign, not just modified. The old single buzz page was completely replaced with a new buzzes (plural) listing page with year-based navigation and metadata structure.

Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。

Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/product.rb:59-61
Timestamp: 2025-09-11T14:47:53.256Z
Learning: Rails アップグレードPRでは、アップグレードに直接関連しない性能改善や機能追加の提案は避けるべき。PRのスコープを維持することが重要で、そのような改善提案は別PRで対応すべきである。

Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。

Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。

head :ok
end
end
6 changes: 0 additions & 6 deletions app/models/question_auto_closer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,18 +69,12 @@ def system_message?(question, system_user, pattern)

def close_with_best_answer(question, system_user)
ActiveRecord::Base.transaction do
remove_existing_best_answer(question)
close_answer = create_close_message(question, system_user)
select_as_best_answer(close_answer)
publish_events(close_answer)
end
end

def remove_existing_best_answer(question)
existing_correct = CorrectAnswer.find_by(question_id: question.id)
existing_correct&.destroy
end

def create_close_message(question, system_user)
question.answers.create!(
user: system_user,
Expand Down
7 changes: 7 additions & 0 deletions db/fixtures/answers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,10 @@ answer<%= i %>:
user: komagata
question: question1
<% end %>

answer51:
description: <%= QuestionAutoCloser::AUTO_CLOSE_WARNING_MESSAGE %>
user: pjord
question: question51
created_at: <%= Time.current - 1.week %>
updated_at: <%= Time.current - 1.week %>
8 changes: 8 additions & 0 deletions db/fixtures/questions.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,11 @@ question<%= i %>:
created_at: "2022-01-15"
published_at: "2022-01-15"
<% end %>

question51:
title: 自動クローズされる質問
description: 自動クローズされる質問です。
user: kimura
practice: practice1
created_at: <%= Time.current - 1.month - 1.week %>
published_at: <%= Time.current - 1.month - 1.week %>