Issue 161: Improve indexing error report and fix Tika failure handling - #162
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the search_elastic Moodle plugin by improving the indexing errors report (adding contextual deep links) and by making Tika failure handling more visible/retriable during file indexing runs.
Changes:
- Add a new “Links” column to the indexing errors system report, rendering course/activity/file links from row context.
- Cache Tika server readiness per PHP process and move the availability check into
analyze_file()so unreachable Tika is recorded as an error. - Add ConnectException handling in the shared HTTP request wrapper.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| version.php | Bumps plugin version (and should keep release/build metadata consistent). |
| tests/externallib_test.php | Adds an extra sleep(1) during teardown. |
| lang/en/search_elastic.php | Adds the links language string for the new report column. |
| classes/reportbuilder/local/systemreports/errors.php | Adds the links column to the default report columns. |
| classes/reportbuilder/local/entities/error.php | Implements the links column rendering and supporting URL resolution helpers. |
| classes/esrequest.php | Adds ConnectException handling (currently risky for Elasticsearch callers). |
| classes/enrich/text/tika.php | Caches Tika readiness per process and throws when Tika is unavailable during analysis. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function analyze_file($file) { | ||
| $filetext = ''; | ||
| if (!$this->tika_server_ready()) { | ||
| throw new Exception('Tika server unavailable'); |
There was a problem hiding this comment.
this is a very generic error. Can you imagine trying to figure what's going on. Would be nice to improve this as well by adding extra details like response code or any error returned.
No need to do it as part of this PR. Can you please create a new issue instead?
| $url = $hostname . ':' . $port; | ||
| private function tika_server_ready(): bool { | ||
| static $ready = null; | ||
| if ($ready !== null) { |
There was a problem hiding this comment.
Hi @dmitriim I'm thinking if initially the Tika server is not available then the $ready = false value will be cached and Tika might recover mid-run too. What if we only cache when it's true so that it can keep retrying if it's false. What do you think? 🤔
There was a problem hiding this comment.
this depends on what you are trying to avoid/fix here
There was a problem hiding this comment.
Hmm.. the original behaviour is if Tika server is down, the scheduled task will crash lol..
Execute scheduled task: Global search indexing (core\task\search_index_task)
... started 14:34:27. Current memory use 22.3 MB.
Debugging increased temporarily due to faildelay of 3840
Processing area: Courses
Processed 20 records containing 20 documents (0 batches), in 0.4 seconds.
Processing area: Course custom fields
No new documents to index.
Processing area: Messages - received
No new documents to index.
Processing area: Course Teacher
No new documents to index.
Processing area: Assignment - activity information
Processed 14 records containing 14 documents (0 batches), in 0.5 seconds.
Processing area: BigBlueButton - activity information
No new documents to index.
Processing area: BigBlueButton - tags information
No new documents to index.
Processing area: Chat - activity information
No new documents to index.
Processing area: Database - entries
No new documents to index.
Processing area: Glossary - entries
No new documents to index.
Processing area: Open Forum - posts
No new documents to index.
Processing area: IMS content package - resource information
No new documents to index.
Processing area: Questionnaire - questions
Processed 1 records containing 1 documents (0 batches), in 0.1 seconds.
Processing area: Survey - activity information
No new documents to index.
Processing area: Wiki - activity information
No new documents to index.
Processing area: Wiki - collaborative pages
No new documents to index.
Processing area: Workshop - activity information
No new documents to index.
Processing area: Users
No new documents to index.
Processing area: File
... used 2096 dbqueries
... used 8.8579370975494 seconds
Scheduled task failed: Global search indexing (core\task\search_index_task),cURL error 7: Failed to connect to 172.24.0.2 port 9998 after 3067 ms: No route to host (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://172.24.0.2:9998
Backtrace:
* line 158 of /lib/guzzlehttp/guzzle/src/Handler/CurlFactory.php: call to GuzzleHttp\Handler\CurlFactory::createRejection()
* line 110 of /lib/guzzlehttp/guzzle/src/Handler/CurlFactory.php: call to GuzzleHttp\Handler\CurlFactory::finishError()
* line 47 of /lib/guzzlehttp/guzzle/src/Handler/CurlHandler.php: call to GuzzleHttp\Handler\CurlFactory::finish()
* line 28 of /lib/guzzlehttp/guzzle/src/Handler/Proxy.php: call to GuzzleHttp\Handler\CurlHandler->__invoke()
* line 48 of /lib/guzzlehttp/guzzle/src/Handler/Proxy.php: call to GuzzleHttp\Handler\Proxy::GuzzleHttp\Handler\{closure}()
* line 54 of /search/engine/elastic/classes/guzzle_helper.php: call to GuzzleHttp\Handler\Proxy::GuzzleHttp\Handler\{closure}()
* line 35 of /lib/guzzlehttp/guzzle/src/PrepareBodyMiddleware.php: call to search_elastic\guzzle_helper::search_elastic\{closure}()
* line 31 of /lib/guzzlehttp/guzzle/src/Middleware.php: call to GuzzleHttp\PrepareBodyMiddleware->__invoke()
* line 71 of /lib/guzzlehttp/guzzle/src/RedirectMiddleware.php: call to GuzzleHttp\Middleware::GuzzleHttp\{closure}()
* line 66 of /lib/guzzlehttp/guzzle/src/Middleware.php: call to GuzzleHttp\RedirectMiddleware->__invoke()
* line 75 of /lib/guzzlehttp/guzzle/src/HandlerStack.php: call to GuzzleHttp\Middleware::GuzzleHttp\{closure}()
* line 333 of /lib/guzzlehttp/guzzle/src/Client.php: call to GuzzleHttp\HandlerStack->__invoke()
* line 107 of /lib/guzzlehttp/guzzle/src/Client.php: call to GuzzleHttp\Client->transfer()
* line 124 of /lib/guzzlehttp/guzzle/src/Client.php: call to GuzzleHttp\Client->sendAsync()
* line 119 of /search/engine/elastic/classes/esrequest.php: call to GuzzleHttp\Client->send()
* line 142 of /search/engine/elastic/classes/esrequest.php: call to search_elastic\esrequest->http_action()
* line 126 of /search/engine/elastic/classes/enrich/text/tika.php: call to search_elastic\esrequest->get()
* line 148 of /search/engine/elastic/classes/enrich/text/tika.php: call to search_elastic\enrich\text\tika->tika_server_ready()
* line 244 of /search/engine/elastic/classes/document.php: call to search_elastic\enrich\text\tika->can_analyze()
* line 604 of /search/engine/elastic/classes/engine.php: call to search_elastic\document->export_file_for_engine()
* line 784 of /search/engine/elastic/classes/engine.php: call to search_elastic\engine->process_document_files()
* line 1309 of /search/classes/manager.php: call to search_elastic\engine->add_documents()
* line 59 of /lib/classes/task/search_index_task.php: call to core_search\manager->index()
* line 408 of /lib/classes/cron.php: call to core\task\search_index_task->execute()
* line 200 of /admin/cli/scheduled_task.php: call to core\cron::run_inner_scheduled_task()
But now it's fixed and we are logging it properly. Anyway, I still cannot decide if it's safer to just remove the caching and keep the original behaviour of checking the server is ready per file. I think we need a smarter way to detect the failure and recover but might be too much for this PR? haha
There was a problem hiding this comment.
What issue are you trying to solve by introducing the cache?
There was a problem hiding this comment.
The cache is mainly for efficiency. Without it, tika_server_ready() makes an HTTP GET to Tika on every file processed, which can add up for large batches.
There was a problem hiding this comment.
I think it's nice to have but the implementation and pros and cons of tika server recovering mid-run and we are not detecting it. I think I will just remove the caching part and keep the fix where Tika server failure is caught instead of entire scheduled task failing. If performance becomes a problem in the future, maybe we can think about better solutions then.
There was a problem hiding this comment.
I removed it and forced pushed. Thank you :)
6bc3b98 to
ebd3905
Compare
Each error row now shows direct links to the related course, activity, or file, eliminating the need to manually look up the affected content when investigating indexing failures.
When Tika was unavailable, can_analyze() returned false silently, so no error was recorded and affected files were permanently stuck in the index with empty content. The retry mechanism never knew to revisit them. Moving the server check into analyze_file() means it throws like any other extraction failure, which triggers the existing error recording and retry path.
…lookup with indexed JOIN
Summary of changes
Add deep links to the indexing errors report
Each row in the indexing errors report now shows direct links to the related course, activity, and file. Previously, investigating a failure required manually looking up the context from the document ID. Links are resolved via SQL JOINs on the report query rather than per-row DB lookups, so the report remains performant at scale.
Record Tika errors when the server is unreachable
Previously, when Tika was unavailable (e.g. wrong hostname or network issue), can_analyze() silently returned false. No error was recorded, affected files were left with empty content in the index, and the retry mechanism never knew to revisit them. The server check is now inside analyze_file(), so an unreachable Tika throws like any other extraction failure. The existing error recording and retry path picks it up automatically.
Testing Instructions
/_search?q=title:<filename>that files are present in the index even when Tika failed, with filetext empty and title set to the filename. For example:Closes #161