From fb5af2890f8aa2b7decedc209639cacffd230b19 Mon Sep 17 00:00:00 2001 From: 128Na Date: Thu, 20 Aug 2026 19:22:37 +0900 Subject: [PATCH 1/2] fix(scrape): isolate site failures in ScrapeAction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Japan の一覧取得(?cmd=list)が接続タイムアウトで失敗すると、ScrapeAction の foreach に try/catch が無いため例外が伝播し、Twitrans/Portal のスク レイピングも丸ごとスキップされていた(prod で8月中旬以降ほぼ毎日発生)。 サイト単位で try/catch し、1サイトの失敗が他サイトを止めないようにする。 Co-Authored-By: Claude Sonnet 5 --- app/Actions/Scrape/ScrapeAction.php | 8 +++++-- docs/known-risks.md | 2 +- .../Actions/Scrape/ScrapeActionTest.php | 22 +++++++++++++++++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/app/Actions/Scrape/ScrapeAction.php b/app/Actions/Scrape/ScrapeAction.php index 56e4e42..e1011cd 100644 --- a/app/Actions/Scrape/ScrapeAction.php +++ b/app/Actions/Scrape/ScrapeAction.php @@ -17,8 +17,12 @@ public function __invoke(?SiteName $siteName, LoggerInterface $logger): void { $siteNames = $siteName instanceof SiteName ? [$siteName] : SiteName::cases(); - foreach ($this->handlerFactory->create($siteNames) as $handler) { - $handler($logger); + foreach ($this->handlerFactory->create($siteNames) as $index => $handler) { + try { + $handler($logger); + } catch (\Throwable $th) { + $logger->error('site failed', [$siteNames[$index]->value, $th]); + } } } } diff --git a/docs/known-risks.md b/docs/known-risks.md index bcec316..e307aba 100644 --- a/docs/known-risks.md +++ b/docs/known-risks.md @@ -12,7 +12,7 @@ | ID | 保護すべき挙動 / Expected Outcome | 制御 | テスト | Status | 是正条件 | 記録日 | |----|----------------------------------|------|--------|--------|----------|--------| -| A1 | 1 サイト/1URL の失敗で他サイト・他 URL の処理が止まらない | Preventive（ハンドラ毎の try/catch） | 2（`Extract/Japan/HandlerIsolationTest` + `Scrape/Japan/HandlerIsolationTest`） | 🟢 OK | — | 2026-06-22 | +| A1 | 1 サイト/1URL の失敗で他サイト・他 URL の処理が止まらない | Preventive（ハンドラ毎の try/catch + `ScrapeAction` でのサイト毎 try/catch） | 3（`Extract/Japan/HandlerIsolationTest` + `Scrape/Japan/HandlerIsolationTest` + `Scrape/ScrapeActionTest::test_one_site_failure_does_not_stop_other_sites`） | 🟢 OK | — | 2026-06-22（2026-08-20 是正: `ScrapeAction` に一覧取得段階の失敗が他サイトを巻き込む欠落を発見、try/catch追加。`ExtractAction` にも同型の欠落が残存、未対応） | | A2 | HTTP 失敗時に RawPage を空/部分 HTML で上書きしない | Preventive（`FetchHtml` で `Http::get()->throw()`。非2xx も例外として扱い upsert に到達しない。4xx は解消しないため即失敗、5xx/接続エラーのみ `retry()` で再試行） | 5（`FetchHtmlTest::test_throws_on_non_2xx...` + `test_does_not_retry_on_4xx...` + `test_retries_on_5xx...` + `Scrape/Japan/HandlerFailureTest`×2） | 🟢 OK | — | 2026-06-22 | | A3 | 同一 RawPage に extract を再実行しても Page 重複が出ない（冪等） | Preventive（`pages_url_unique` + HasOne） | 2（`Extract/UpdateOrCreatePageTest`） | 🟢 OK | — | 2026-06-22 | | A4 | 抽出失敗時にスクレイプ済データ（RawPage）を破壊しない | Preventive（`MarkExtractFailed`：削除せず `extract_failed_at` で隔離、成功時にクリア。`extract_failed_at` に index 追加済み） | 4（`MarkExtractFailedTest`×3 + `HandlerIsolationTest`） | 🟢 OK | — | 2026-06-22 | diff --git a/tests/Feature/Actions/Scrape/ScrapeActionTest.php b/tests/Feature/Actions/Scrape/ScrapeActionTest.php index a73d3d6..942d32e 100644 --- a/tests/Feature/Actions/Scrape/ScrapeActionTest.php +++ b/tests/Feature/Actions/Scrape/ScrapeActionTest.php @@ -9,6 +9,7 @@ use App\Actions\Scrape\ScrapeAction; use App\Enums\SiteName; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Http\Client\ConnectionException; use Illuminate\Support\Facades\Http; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; @@ -51,4 +52,25 @@ public function test_invokes_specific_handler_when_site_provided(): void Http::assertSent(fn ($request): bool => str_contains((string) $request->url(), 'japanese.simutrans.com')); Http::assertNotSent(fn ($request): bool => str_contains((string) $request->url(), 'wikiwiki.jp/twitrans')); } + + public function test_one_site_failure_does_not_stop_other_sites(): void + { + Http::fake([ + 'https://japanese.simutrans.com?cmd=list' => fn (): never => throw new ConnectionException('connection failed'), + '*' => Http::response('', 200), + ]); + + // Mock PortalHandler to avoid database queries in CI where the portal connection is unmigrated + $this->app->bind(Handler::class, fn (): HandlerInterface => new class implements HandlerInterface + { + public function __invoke(LoggerInterface $logger): void {} + }); + + $scrapeAction = app(ScrapeAction::class); + $scrapeAction(null, new NullLogger); + + // Japan's list fetch fails entirely, but Twitrans must still run. + Http::assertNotSent(fn ($request): bool => str_contains((string) $request->url(), 'japanese.simutrans.com/index.php')); + Http::assertSent(fn ($request): bool => str_contains((string) $request->url(), 'wikiwiki.jp/twitrans')); + } } From 868463831493aa931de75d95e3120f047b8af1c1 Mon Sep 17 00:00:00 2001 From: 128Na Date: Thu, 20 Aug 2026 19:30:29 +0900 Subject: [PATCH 2/2] fix(scrape): rethrow after all sites attempted to keep alerting intact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit コードレビュー指摘への対応。前回の修正(サイト毎 try/catch)は例外を ScrapeAction 内で握りつぶしていたため、ScrapeCommand の catch まで 届かなくなり、report() 経由のDiscord通知・失敗時の終了コード・ last_crawl キャッシュ更新スキップが働かなくなっていた。 全サイトを試行した後、失敗が1件でもあれば最後に再送出することで、 「1サイトの失敗が他サイトを止めない」を保ったまま、失敗の可視化 (Discord通知/終了コード/last_crawl据え置き)を復元する。 Co-Authored-By: Claude Sonnet 5 --- app/Actions/Scrape/ScrapeAction.php | 10 ++++++++++ docs/known-risks.md | 2 +- tests/Feature/Actions/Scrape/ScrapeActionTest.php | 12 ++++++++++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/Actions/Scrape/ScrapeAction.php b/app/Actions/Scrape/ScrapeAction.php index e1011cd..a2a84e2 100644 --- a/app/Actions/Scrape/ScrapeAction.php +++ b/app/Actions/Scrape/ScrapeAction.php @@ -17,12 +17,22 @@ public function __invoke(?SiteName $siteName, LoggerInterface $logger): void { $siteNames = $siteName instanceof SiteName ? [$siteName] : SiteName::cases(); + $failure = null; + foreach ($this->handlerFactory->create($siteNames) as $index => $handler) { try { $handler($logger); } catch (\Throwable $th) { $logger->error('site failed', [$siteNames[$index]->value, $th]); + $failure ??= $th; } } + + // 全サイトを試行した後で改めて投げ直す。呼び出し元(ScrapeCommand)の + // report()/終了コード/last_crawl 更新スキップが、1サイトの失敗でも + // 引き続き働くようにするため。 + if ($failure instanceof \Throwable) { + throw $failure; + } } } diff --git a/docs/known-risks.md b/docs/known-risks.md index e307aba..1b6bc07 100644 --- a/docs/known-risks.md +++ b/docs/known-risks.md @@ -12,7 +12,7 @@ | ID | 保護すべき挙動 / Expected Outcome | 制御 | テスト | Status | 是正条件 | 記録日 | |----|----------------------------------|------|--------|--------|----------|--------| -| A1 | 1 サイト/1URL の失敗で他サイト・他 URL の処理が止まらない | Preventive（ハンドラ毎の try/catch + `ScrapeAction` でのサイト毎 try/catch） | 3（`Extract/Japan/HandlerIsolationTest` + `Scrape/Japan/HandlerIsolationTest` + `Scrape/ScrapeActionTest::test_one_site_failure_does_not_stop_other_sites`） | 🟢 OK | — | 2026-06-22（2026-08-20 是正: `ScrapeAction` に一覧取得段階の失敗が他サイトを巻き込む欠落を発見、try/catch追加。`ExtractAction` にも同型の欠落が残存、未対応） | +| A1 | 1 サイト/1URL の失敗で他サイト・他 URL の処理が止まらない | Preventive（ハンドラ毎の try/catch + `ScrapeAction` でのサイト毎 try/catch。全サイト試行後、失敗があれば1件だけ再送出し、呼び出し元(`ScrapeCommand`)の `report()`/終了コード/`last_crawl` 更新スキップを維持） | 3（`Extract/Japan/HandlerIsolationTest` + `Scrape/Japan/HandlerIsolationTest` + `Scrape/ScrapeActionTest::test_one_site_failure_does_not_stop_other_sites`） | 🟢 OK | — | 2026-06-22（2026-08-20 是正: `ScrapeAction` に一覧取得段階の失敗が他サイトを巻き込む欠落を発見、try/catch追加。当初案はDiscord通知/`last_crawl`更新スキップも巻き添えで失っていたため、全サイト試行後に再送出する形に修正。`ExtractAction` にも同型の欠落が残存、未対応） | | A2 | HTTP 失敗時に RawPage を空/部分 HTML で上書きしない | Preventive（`FetchHtml` で `Http::get()->throw()`。非2xx も例外として扱い upsert に到達しない。4xx は解消しないため即失敗、5xx/接続エラーのみ `retry()` で再試行） | 5（`FetchHtmlTest::test_throws_on_non_2xx...` + `test_does_not_retry_on_4xx...` + `test_retries_on_5xx...` + `Scrape/Japan/HandlerFailureTest`×2） | 🟢 OK | — | 2026-06-22 | | A3 | 同一 RawPage に extract を再実行しても Page 重複が出ない（冪等） | Preventive（`pages_url_unique` + HasOne） | 2（`Extract/UpdateOrCreatePageTest`） | 🟢 OK | — | 2026-06-22 | | A4 | 抽出失敗時にスクレイプ済データ（RawPage）を破壊しない | Preventive（`MarkExtractFailed`：削除せず `extract_failed_at` で隔離、成功時にクリア。`extract_failed_at` に index 追加済み） | 4（`MarkExtractFailedTest`×3 + `HandlerIsolationTest`） | 🟢 OK | — | 2026-06-22 | diff --git a/tests/Feature/Actions/Scrape/ScrapeActionTest.php b/tests/Feature/Actions/Scrape/ScrapeActionTest.php index 942d32e..02b02b5 100644 --- a/tests/Feature/Actions/Scrape/ScrapeActionTest.php +++ b/tests/Feature/Actions/Scrape/ScrapeActionTest.php @@ -67,9 +67,17 @@ public function __invoke(LoggerInterface $logger): void {} }); $scrapeAction = app(ScrapeAction::class); - $scrapeAction(null, new NullLogger); - // Japan's list fetch fails entirely, but Twitrans must still run. + // Japan's list fetch fails entirely, but Twitrans must still run before + // the failure is rethrown so the caller (ScrapeCommand) can still + // report()/exit non-zero/skip the last_crawl cache update. + try { + $scrapeAction(null, new NullLogger); + $this->fail('Expected ConnectionException was not thrown.'); + } catch (ConnectionException) { + // expected + } + Http::assertNotSent(fn ($request): bool => str_contains((string) $request->url(), 'japanese.simutrans.com/index.php')); Http::assertSent(fn ($request): bool => str_contains((string) $request->url(), 'wikiwiki.jp/twitrans')); }