fix_62589 - #1913
fix_62589#1913KotaroInoue1448 wants to merge 1 commit into
Conversation
Reviewer's GuideThe PR reduces repeated Celery health pings for task-related status checks by introducing configurable Redis-backed caching, with graceful Redis failure handling and focused unit/integration test coverage. Sequence diagram for cached Celery status checkssequenceDiagram
participant Admin as Admin status request
participant View as check_export_status
participant Checker as check_celery_is_run
participant Cache as Redis cache
participant Celery as Celery inspect
Admin->>View: check_export_status()
View->>Checker: check_celery_is_run(is_task=True)
Checker->>Cache: get_redis_cache(cache_key)
alt cached status is available
Cache-->>Checker: status 1 or 0
Checker-->>View: True or False
else cache miss
Checker->>Celery: inspect(timeout).ping()
Celery-->>Checker: worker status
Checker->>Cache: datastore.put(cache_key, status, cache_ttl)
Checker-->>View: worker status
end
View-->>Admin: export status response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/weko-search-ui/weko_search_ui/tasks.py" line_range="326-331" />
<code_context>
+ "WEKO_SEARCH_UI_CELERY_STATUS_CACHE_TTL", 60
+ ))
+
+ cached_status = get_redis_cache(cache_key)
+ if cached_status and is_task:
+ if cached_status == "1":
+ return True
</code_context>
<issue_to_address>
**nitpick (performance):** `check_celery_is_run(is_task=False)` performs a Redis cache lookup even though the result is ignored whenever `is_task` is false, so every non-task status check now adds an unnecessary Redis round trip before performing the live Celery ping.
**Triggers:** When callers such as reindex/import checks or the bulk-export submission endpoint invoke `check_celery_is_run()` without `is_task=True`.
**Suggested fix:** Move `get_redis_cache(cache_key)` inside the `if is_task:` branch so non-task callers retain the original live-ping path without contacting Redis.
```suggestion
if is_task:
cached_status = get_redis_cache(cache_key)
if cached_status:
if cached_status == "1":
return True
elif cached_status == "0":
return False
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. The change caches the Celery worker ping result and can make export-status checks use a stale worker state for up to 60 seconds, causing a bounded incorrect status or delay. Reverting stops using the cache, and the cached value expires or can be recomputed, so the effect is repairable.
| cached_status = get_redis_cache(cache_key) | ||
| if cached_status and is_task: | ||
| if cached_status == "1": | ||
| return True | ||
| elif cached_status == "0": | ||
| return False |
There was a problem hiding this comment.
nitpick (performance): check_celery_is_run(is_task=False) performs a Redis cache lookup even though the result is ignored whenever is_task is false, so every non-task status check now adds an unnecessary Redis round trip before performing the live Celery ping.
Triggers: When callers such as reindex/import checks or the bulk-export submission endpoint invoke check_celery_is_run() without is_task=True.
Suggested fix: Move get_redis_cache(cache_key) inside the if is_task: branch so non-task callers retain the original live-ping path without contacting Redis.
| cached_status = get_redis_cache(cache_key) | |
| if cached_status and is_task: | |
| if cached_status == "1": | |
| return True | |
| elif cached_status == "0": | |
| return False | |
| if is_task: | |
| cached_status = get_redis_cache(cache_key) | |
| if cached_status: | |
| if cached_status == "1": | |
| return True | |
| elif cached_status == "0": | |
| return False |
1 participant
概要 (Summary)
celeryステータス取得メソッドのキャッシュ化
関連Issue / チケット (Related Issues)
https://redmine.devops.rcos.nii.ac.jp/issues/62589
変更タイプ (Type of Change)
🤖 0. CI 自動チェック (API Inventory Drift)
PR ごとに WEKO3 コンテナを起動し、
url_mapのダンプ・台帳との突き合わせ・変更行の到達可否測定を自動実行する。結果は PR コメントと Actions の artifact
(
api-inventory-summary) に出る。このリポジトリは public のため、台帳もベースラインも同梱していない。
実データはプライベートリポジトリ
RCOSDP/weko-secretにあり、CI は Secret 経由で取得する。以降この文書では、そこを単にプライベートリポジトリと呼ぶ。
台帳はブランチごとに内容が違うため、CI は weko 側と同名のブランチを
プライベートリポジトリから探して使う（head → base → 既定ブランチ の順）。
採用されたブランチ名は PR コメントの冒頭に出るので、件数を読む前にそこを見ること。
対応ブランチが無い場合は既定ブランチと比較され、コメント冒頭に警告が出る。
その件数は当てにならないので、PASS でも「確認済み」と読まないこと。
詳細:
tools/api-inventory/ci/README.md§3aSecret (
API_INVENTORY_REPO/API_INVENTORY_SSH_KEY) が未設定のリポジトリ、および fork からの PR では、このジョブは何もせずスキップされる。
API を追加・変更した場合（必須）
この PR が
fix/issue62569→develop_v2.0.4なら、プライベート側もfix/issue62569→develop_v2.0.4。同名にしておけば台帳 PR が未マージでもCI がそれを見るので、2つの PR のマージ順を気にしなくてよい。
api_snapshot.jsonを更新し、対応する PR を出したbash export WEKO_API_INVENTORY_DIR=/path/to/weko-secret ./install.sh python3 tools/api-inventory/scripts/snapshot.py --out "$WEKO_API_INVENTORY_DIR/api_snapshot.json"更新しないと CI が落ちる。公開リポジトリのコード変更とは別の PRになる。
weko3_api_list_full.tsvに行を追加・更新し、build_checklist.pyで 24 列版を再生成した（未収載だと reconcile が FAIL する）（
git statusに*.tsv/api_snapshot.jsonが出ていないこと）FAIL したときの対処（要約）
まず PR コメント冒頭の台帳ブランチを見る。警告が出ていれば、件数を追う前に
プライベートリポジトリ側の対応ブランチを用意すること（比較相手が違うので件数に意味がない）。
ジョブが落ちる条件は 3 つある。PR コメントのどのセクションに件数が出ているかで切り分ける。
drift.md)reconcile.md)*_PERMISSION_FACTORY/ CSRF 保護 等が危険側の値に変わったcan_delete/can_exportがFalse→Truedata_opを更新data_opが作成/更新/削除の経路に、未認証で到達したdata_opの記載誤りなら台帳を直すurl_mapに無いreconcile B のうち、実機に存在しないことが正当な行（プラグイン未登録・config で無効等）は
プライベートリポジトリの
reconcile_allow.jsonに理由付きで登録する。理由なしの登録は不可。登録済みの行は B'（既知・許容）として集計され、E'（endpoint が実機に無い）と併せてゲート対象外になる。
W1〜W6 は WARN でゲートは通るが、レビューでは見ること
（ModelView の追加 / 実装本体の変化 / HTTP メソッド・URL の変化 / 監視対象 config の変化 /
依存パッケージの版の変化）。特に W6（依存の版）は、ベースラインを CI と異なる環境で作ると
毎回出続けて形骸化するため、ベースラインは
install.shで作った環境から生成する。🔒 1. セキュリティ & API アクセス制御チェック (必須)
認証・認可 (Authentication & Authorization)
@login_required,@pass_record,need(...), Invenio Access Action/api/*ではPermission.require(http_exception=403)を使うこと。@login_requiredは API アプリにsecurity.loginが無いため 401 ではなく 500 になる--allow-writes付きでGET / HEAD 以外も叩く）
起動した経路のみ。ワークフロー系など未解決プレースホルダの行は skip される
Noneで無効化していない*_PERMISSION_FACTORY等を監視機能クローズ・非公開化の場合 (Feature Disable)
404 Not Foundまたは403 Forbiddenが返ることを確認した🧪 2. テストコード観点チェック (pytest / Invenio Test Suite)
権限・異常系テスト (Negative & Authorization Tests)
401 Unauthorizedまたは403 Forbidden/404 Not Foundが返ることを検証するテストがある403になるテストがある404/403を返すテストがある境界値・入力バリデーションテスト (Boundary & Validation)
400 Bad Request/ バリデーションエラーが返るテストがあるデータ整合性・トランザクションテスト (Integrity & Rollback)
🛡️ 3. データ保護 & 破壊的変更防止チェック (Data Safety)
⚙️ 4. マイグレーション & システム影響チェック (Invenio / WEKO3 Stack)
データベース (DB / Alembic)
invenio alembic upgrade（適用）およびdowngrade（ロールバック）スクリプトを作成・検証した検索インデックス (Elasticsearch / OpenSearch)
設定 & 非同期処理 (Config / Celery / Cache)
invenio.cfg/ 環境変数のデフォルト値を設定した📚 5. ドキュメント・仕様書更新チェック (weko-document)
tools/api-inventory/、台帳・調査記録はプライベートリポジトリ（public リポジトリには置かない）。
§0 のチェック項目で対応済みなら、ここは確認のみ。
weko3_api_auth_findings.md）もプライベートリポジトリに置く。台帳は二重管理しない📋 6. 動作検証エビデンス (Verification Evidence)
テスト実行結果
(invenio) invenio@ffc19f18cf6f:/code/modules/weko-search-ui$ .tox/c1/bin/pytest --cov=weko_search_ui tests/test_admin.py::TestItemBulkExport::test_check_export_status -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-search-ui/.tox/c1/tmp
================================================================================== test session starts ===================================================================================
platform linux -- Python 3.6.15, pytest-6.1.2, py-1.11.0, pluggy-0.13.1 -- /code/modules/weko-search-ui/.tox/c1/bin/python
cachedir: .pytest_cache
rootdir: /code/modules/weko-search-ui, configfile: tox.ini
plugins: celery-4.4.4, mock-3.6.1, cov-4.0.0
collected 1 item
tests/test_admin.py::TestItemBulkExport::test_check_export_status PASSED
(invenio) invenio@ffc19f18cf6f:/code/modules/weko-search-ui$ .tox/c1/bin/pytest --cov=weko_search_ui tests/test_tasks.py::test_check_celery_is_run -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-search-ui/.tox/c1/tmp
================================================================================== test session starts ===================================================================================
platform linux -- Python 3.6.15, pytest-6.1.2, py-1.11.0, pluggy-0.13.1 -- /code/modules/weko-search-ui/.tox/c1/bin/python
cachedir: .pytest_cache
rootdir: /code/modules/weko-search-ui, configfile: tox.ini
plugins: celery-4.4.4, mock-3.6.1, cov-4.0.0
collected 1 item
tests/test_tasks.py::test_check_celery_is_run PASSED
CI の成果物 (artifact:
api-inventory-summary)drift.mdreconcile.md明細（該当した経路名・実測結果）は公開できないため artifact に含めていない。
プライベートリポジトリ側で同じコマンドを
--summary-onlyなしで実行して確認する。手動で確認したこと
Summary by Sourcery
Cache Celery worker status checks to reduce repeated health-check requests.
Bug Fixes:
Enhancements:
Tests: