DifyのPersistent StorageのGC機能の追加 #19
Conversation
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant User as Slack User
participant Slack as Slack
participant Endpoint as SlackEndpoint
participant Storage as Storage
User->>Slack: @bot delcache コマンド送信
Slack->>Endpoint: app_mentionイベント
Endpoint->>Storage: キャッシュキー一覧取得
Endpoint->>Storage: 対象キャッシュ削除
alt 削除成功
Endpoint->>Slack: 削除完了メッセージ送信
else 削除失敗
Endpoint->>Slack: エラーメッセージ送信
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
endpoints/slack.py (3)
244-251: 辞書のキー取得を最適化できます
dict.keys()は不要です。より簡潔な書き方に改善できます。-return [k for k in registry.keys() if k != "__last_updated"] +return [k for k in registry if k != "__last_updated"]
407-441: キャッシュ削除コマンドの実装は機能的です
delcacheコマンドは正しくキャッシュとconversationストレージの両方を削除し、適切なフィードバックを提供しています。エラーメッセージ送信部分で
contextlib.suppressを使用すると、より簡潔に書けます：from contextlib import suppress # Instead of try-except-pass with suppress(SlackApiError): client.chat_postMessage( channel=channel, thread_ts=thread_ts, text=f"❌ Error deleting cache: {str(e)}" )
830-850: 緊急クリーンアップ機能は有効ですが、エラー検出を改善できますストレージ上限エラー時の50%クリーンアップは適切な対応です。ただし、文字列マッチングによるエラー検出は脆弱な可能性があります。
エラーコードやエラータイプによる検出を検討してください：
# If the storage library provides specific exception types except StorageLimitExceededError as e: # Handle storage limit specifically except Exception as e: # Handle other errors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
endpoints/slack.py(7 hunks)manifest.yaml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
endpoints/slack.py
248-248: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
429-436: Use contextlib.suppress(SlackApiError) instead of try-except-pass
Replace with contextlib.suppress(SlackApiError)
(SIM105)
842-849: Use contextlib.suppress(SlackApiError) instead of try-except-pass
Replace with contextlib.suppress(SlackApiError)
(SIM105)
🔇 Additional comments (7)
manifest.yaml (1)
36-36: ストレージサイズの増加は適切です1MBから10MBへの増加は、新しいキーレジストリとキャッシュ管理機能を考慮すると妥当な変更です。
endpoints/slack.py (6)
195-196: キャッシュ期間と定期クリーンアップ間隔の設定は適切です24時間のキャッシュ保持期間と定期クリーンアップ間隔は、ストレージ使用量と性能のバランスが取れた設定です。
233-243: レジストリ取得メソッドの実装は堅牢ですエラーハンドリングが適切に実装されており、デフォルト値の返却も問題ありません。
252-284: ストレージクリーンアップの実装は適切です古いキーから削除する実装は論理的で、個別のエラーハンドリングにより部分的な失敗にも対応できています。
295-310: 定期クリーンアップの実装は効率的です24時間ごとの30%クリーンアップは、通常運用において適切なバランスです。
_invokeの開始時にチェックすることで、確実に実行されます。Also applies to: 361-362
328-329: キャッシュメソッドへのレジストリ統合は適切です既存のキャッシュメソッドに
_add_key_to_registryを追加することで、すべてのキーが確実に追跡されます。Also applies to: 352-353
699-699: 会話ストレージキーの登録も適切に実装されていますconversation_idの保存時にキーをレジストリに追加することで、包括的なストレージ管理が実現されています。
| def _add_key_to_registry(self, key: str): | ||
| """キーをレジストリに追加""" | ||
| if key == "__keys__": # 無限ループ防止 | ||
| return | ||
|
|
||
| try: | ||
| # 現在のレジストリを取得 | ||
| registry = self._get_key_registry() | ||
|
|
||
| # キーを追加（既存の場合は時間を更新） | ||
| registry[key] = int(time.time()) # last_update時刻 | ||
| registry["__last_updated"] = int(time.time()) | ||
|
|
||
| # レジストリを保存 | ||
| self.session.storage.set("__keys__", json.dumps(registry).encode("utf-8")) | ||
|
|
||
| except Exception as e: | ||
| print(f"Error adding key to registry: {e}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
レジストリへのキー追加にスレッドセーフティを考慮してください
複数のスレッドが同時にレジストリにアクセスする可能性があるため、競合状態が発生する可能性があります。また、エラーハンドリングがコンソール出力のみなので、本番環境では不十分かもしれません。
スレッドロックの追加を検討してください：
import threading
class SlackEndpoint(Endpoint):
def __init__(self):
self._registry_lock = threading.Lock()
def _add_key_to_registry(self, key: str):
"""キーをレジストリに追加"""
if key == "__keys__":
return
with self._registry_lock:
try:
# 現在のレジストリを取得
registry = self._get_key_registry()
# ... rest of the code🤖 Prompt for AI Agents
In endpoints/slack.py around lines 198 to 216, the _add_key_to_registry method
is not thread-safe and only prints errors to the console. To fix this, add a
threading.Lock instance (e.g., self._registry_lock) in the class constructor to
synchronize access to the registry. Then, wrap the registry access and update
code inside a with self._registry_lock block to prevent race conditions. Also,
replace the print statement with proper logging or error handling suitable for
production environments.
… comments and docstrings from Japanese to English.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
endpoints/slack.py (2)
361-363: 日本語コメントを英語に変更してください過去のレビューで指摘されたとおり、Dify Plugin Docsでは、ソースコード内での非英語テキストの使用は推奨されていません。
- # 定期クリーンアップチェック + # Periodic cleanup check
198-251: レジストリメソッドのスレッドセーフティとエラーハンドリングの改善が必要過去のレビューで指摘されたスレッドセーフティの問題が未解決です。また、エラーハンドリングがconsole出力のみで本番環境には不適切です。
スレッドロックとログ改善の提案：
+import threading +import logging class SlackEndpoint(Endpoint): + def __init__(self): + super().__init__() + self._registry_lock = threading.Lock() + def _add_key_to_registry(self, key: str): """Add key to registry""" if key == "__keys__": return + with self._registry_lock: try: # Get current registry registry = self._get_key_registry() # ... rest of the code except Exception as e: - print(f"Error adding key to registry: {e}") + logging.error(f"Error adding key to registry: {e}")
🧹 Nitpick comments (5)
endpoints/slack.py (5)
252-310: クリーンアップロジックは適切ですが、ログ出力の改善を推奨ストレージクリーンアップの実装は論理的に正しく、古いキーから削除する仕組みも適切です。ただし、エラーハンドリングでprintステートメントを使用しているため、本番環境では適切なロギングシステムに変更することを推奨します。
ログ改善の提案：
- print(f"Error deleting key {key_to_delete}: {e}") + logging.error(f"Error deleting key {key_to_delete}: {e}") - print(f"Storage cleanup completed: deleted {keys_to_delete} keys out of {total_keys}") + logging.info(f"Storage cleanup completed: deleted {keys_to_delete} keys out of {total_keys}") - print("Periodic storage cleanup completed") + logging.info("Periodic storage cleanup completed")
248-248: 辞書の反復処理を最適化してください静的解析ツールの指摘通り、
dict.keys()を使用せずに辞書を直接反復処理する方が効率的です。- return [k for k in registry.keys() if k != "__last_updated"] + return [k for k in registry if k != "__last_updated"]
407-441: delcacheコマンドの実装が適切です手動キャッシュ削除機能の実装は論理的に正しく、ユーザーフィードバックも適切に提供されています。静的解析ツールの提案に従って、エラーハンドリングを最適化することを推奨します。
contextlib.suppressを使用した最適化：
+from contextlib import suppress - try: - client.chat_postMessage( - channel=channel, - thread_ts=thread_ts, - text=f"❌ Error deleting cache: {str(e)}" - ) - except SlackApiError: - pass + with suppress(SlackApiError): + client.chat_postMessage( + channel=channel, + thread_ts=thread_ts, + text=f"❌ Error deleting cache: {str(e)}" + )
825-850: 緊急時クリーンアップ機能が適切に実装されていますストレージサイズ制限エラーの検出と50%クリーンアップの実行、ユーザー通知の仕組みが適切に実装されています。静的解析ツールの提案に従って最適化を推奨します。
contextlib.suppressを使用した最適化：
+from contextlib import suppress - try: - client.chat_postMessage( - channel=channel, - thread_ts=thread_ts, - text="⚠️ Storage cleanup failed. Please contact administrator.", - ) - except SlackApiError: - pass + with suppress(SlackApiError): + client.chat_postMessage( + channel=channel, + thread_ts=thread_ts, + text="⚠️ Storage cleanup failed. Please contact administrator.", + )
194-891: 全体的なアーキテクチャが優秀です包括的なキャッシュ管理システムの実装により、PRの目的である自動GC機能と手動キャッシュ削除機能が適切に実現されています。既存コードとの統合も適切で、ユーザーフィードバック機能も充実しています。
主な改善点：
- スレッドセーフティの追加
- ログシステムの改善
- 静的解析の最適化提案の適用
- 日本語コメントの英語化
本実装は、ストレージ容量制限の問題を効果的に解決し、プラグインの安定性と保守性を大幅に向上させる優秀な設計です。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
endpoints/slack.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
endpoints/slack.py
248-248: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
429-436: Use contextlib.suppress(SlackApiError) instead of try-except-pass
Replace with contextlib.suppress(SlackApiError)
(SIM105)
842-849: Use contextlib.suppress(SlackApiError) instead of try-except-pass
Replace with contextlib.suppress(SlackApiError)
(SIM105)
🔇 Additional comments (2)
endpoints/slack.py (2)
194-197: 定数の設定が適切ですキャッシュ保持期間を24時間に延長し、クリーンアップ間隔も同じく24時間に設定されているのは、PRの目的に合致しており適切です。
328-328: キーレジストリの統合ポイントが適切ですキャッシュの読み込み、追加、会話ID保存時にキーをレジストリに追加する処理が適切に統合されています。
Also applies to: 352-352, 699-699
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
endpoints/slack.py (1)
198-251: レジストリ管理の実装について複数の改善点があります
- スレッドセーフティの問題: 過去のレビューで指摘されたように、複数のスレッドが同時にレジストリにアクセスする可能性があり、競合状態が発生する可能性があります。
- エラーハンドリングの改善: 現在はコンソール出力のみですが、本番環境では適切なログ出力やエラー処理が必要です。
- コードスタイルの改善: 静的解析ツールが指摘するように、Line 248で
key in dict.keys()の代わりにkey in dictを使用すべきです。以下の修正を適用してください：
def _get_all_keys(self) -> List[str]: """Get all keys (except __last_updated)""" try: registry = self._get_key_registry() - return [k for k in registry.keys() if k != "__last_updated"] + return [k for k in registry if k != "__last_updated"] except Exception: return []
🧹 Nitpick comments (4)
endpoints/slack.py (4)
252-309: ストレージクリーンアップのロジックは優秀です古いキーから順番に削除する仕組みや、設定可能なクリーンアップ割合など、よく設計されています。ただし、エラーハンドリングがコンソール出力のみなので、本番環境での適切なログ出力を検討してください。
407-441: 手動キャッシュ削除機能が適切に実装されています機能的には優秀ですが、静的解析ツールが指摘するように、try-except-passパターンの改善を検討してください。
以下の改善を適用できます：
+import contextlib # ... in the delcache handling ... - try: - client.chat_postMessage( - channel=channel, - thread_ts=thread_ts, - text=f"❌ Error deleting cache: {str(e)}" - ) - except SlackApiError: - pass + with contextlib.suppress(SlackApiError): + client.chat_postMessage( + channel=channel, + thread_ts=thread_ts, + text=f"❌ Error deleting cache: {str(e)}" + )
830-849: ストレージ制限エラーの処理が重要な機能として適切に実装されています緊急時のクリーンアップ機能は素晴らしい追加機能です。静的解析ツールの提案と同様に、try-except-passパターンの改善を検討してください。
+import contextlib # ... in the storage error handling ... - try: - client.chat_postMessage( - channel=channel, - thread_ts=thread_ts, - text="⚠️ Storage cleanup failed. Please contact administrator.", - ) - except SlackApiError: - pass + with contextlib.suppress(SlackApiError): + client.chat_postMessage( + channel=channel, + thread_ts=thread_ts, + text="⚠️ Storage cleanup failed. Please contact administrator.", + )
194-892: GC機能の全体的なアーキテクチャが優秀ですこの実装は以下の点で優れています：
- ストレージ容量問題を効果的に解決
- 自動・手動両方のクリーンアップ機能
- 既存機能との適切な統合
- レジストリベースのキー管理による効率的な実装
ストレージ管理の課題に対する包括的で実用的なソリューションです。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
endpoints/slack.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
endpoints/slack.py
248-248: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
429-436: Use contextlib.suppress(SlackApiError) instead of try-except-pass
Replace with contextlib.suppress(SlackApiError)
(SIM105)
842-849: Use contextlib.suppress(SlackApiError) instead of try-except-pass
Replace with contextlib.suppress(SlackApiError)
(SIM105)
🔇 Additional comments (2)
endpoints/slack.py (2)
195-197: 定数の変更が適切ですキャッシュ期間の24時間への延長とクリーンアップ間隔の追加により、GC機能が適切にサポートされています。
328-328: レジストリ統合が適切に実装されています既存のメソッドにレジストリへのキー追加が適切に統合されており、すべてのストレージ操作が追跡されるようになっています。
Also applies to: 352-352, 361-363, 699-699
2 participants
機能追加: ストレージ管理とキャッシュ削除機能
Difyプラグインのストレージ容量制限による問題を解決するため、ストレージの自動ガベージコレクション（GC）機能と、手動でのキャッシュ削除機能を追加しました。
主な変更点
1. ストレージ自動クリーンアップ (GC) 機能
2. 手動キャッシュ削除コマンド
delcacheと投稿することで、そのスレッドに関連するキャッシュ（会話履歴など）を手動で削除できるようになりました。3. その他
0.5.0に更新しました。これらの変更により、プラグインの安定性とメンテナンス性が向上します。
Summary by CodeRabbit
新機能
改善