[Bug 43569] New: update_index masks per-document ES failures; reindex reports silent success
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Bug ID: 43569 Summary: update_index masks per-document ES failures; reindex reports silent success Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Searching - Elasticsearch Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Elasticsearch indexing can silently drop records: update_index() masks per-document bulk failures, and rebuild_elasticsearch.pl reports success even when records fail to index. This is a follow-up to bug 42669, which fixed the es_indexer_daemon.pl silent job failure and NoNodes recovery. That fix only handles thrown exceptions (whole-operation failures). It does not address the case where the ES bulk request returns HTTP 200 but individual documents fail (mapping conflicts, strict_dynamic_mapping_exception, field-too-long, etc.). Those per-item failures are currently swallowed. == Current behavior == Koha::SearchEngine::Elasticsearch::Indexer::update_index(): - On a bulk response with $response->{errors} true, it only carp()s "One or more ElasticSearch errors occurred when indexing documents" and returns the raw response as if successful. - Callers cannot easily tell which records failed or act on them. misc/workers/es_indexer_daemon.pl: - Ignores the update_index() return value entirely; relies only on try/catch for exceptions. A partial bulk failure leaves the batch marked 'finished', so the affected records are silently missing from search. misc/search_tools/rebuild_elasticsearch.pl: - The buffered commit is wrapped in try/catch that only logs and continues; dropped records are never retried. - The final commit (uncommitted tail) is NOT wrapped, so under 'use autodie' an exception there kills the process mid-slice. - _handle_response() prints per-item error detail only at verbosity level 2, and even the summary line is suppressed at the default verbosity used by cron. Reindex cronjobs therefore report success while records fail. - Exit status is always 0, so cron wrappers cannot detect failures. - With --processes, child exit status is not propagated (wait() ignores $?), so a failed slice is invisible to the parent. == Proposed change (two-phase implementation on this bug) == Phase 1: Surface per-document bulk failures. - update_index() returns a Koha::Result::Boolean instead of the raw ES response. False when any bulk item errored; one message per failed record via add_message({ type => 'error', message => reason, payload => { record_id => id, error => ... } }). - Empty-body case returns a true Boolean (removes the current undef return and the undef-deref hazard in callers). - Whole-operation failures (bulk call throws, NoNodes) keep throwing Koha::Exceptions::Elasticsearch::BadResponse, so the daemon NoNodes reset path from bug 42669 stays intact. Exceptions mean "could not run"; the Boolean means "ran, some documents failed". - Migrate callers: * es_indexer_daemon.pl: inspect the Boolean; on false set index_ok = 0 so the batch is marked 'failed' instead of 'finished'. NOTE: this makes previously-hidden partial failures visible as failed jobs (intended). * rebuild_elasticsearch.pl: minimal migration so _handle_response does not break on the new return type (full hardening is Phase 2). * index_records() and bulkmarcimport.pl currently ignore the return; left as-is in Phase 1, noted as optional future work. - Tests: update t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t (the existing update_index assertion) and add all-success, partial-failure, empty-body, and whole-op-throws cases. Phase 2: Harden rebuild_elasticsearch.pl (depends on Phase 1). - Track failed/skipped counts; exit non-zero on any failure so cron wrappers can detect trouble. (Visible behavior change for existing automation.) - Wrap the final commit in the same try/catch as the buffered one, honoring the NoNodes-vs-real-error distinction so a hard outage is not masked as a soft warning under autodie. - Propagate child exit status in the --processes path (children exit non-zero on failure; parent aggregates $? from wait()). - Report fetched vs indexed vs failed vs skipped at the end; the current "Total N records indexed" line overstates success. == Out of scope == Standardizing logging across the ES indexing stack (Carp vs Koha::Logger alignment) will be handled in a separate report. == Open questions for QA == - Should index_records() aggregate and return a combined Boolean, or stay fire-and-forget for now? - Does the daemon 'finished' -> 'failed' semantic shift for partial failures warrant a release note? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, | |lisette@bywatersolutions.co | |m, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206318 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206318&action=edit Bug 43569: Phase 1 - update_index reports per-document failures Koha::SearchEngine::Elasticsearch::Indexer::update_index now returns a Koha::Result::Boolean instead of the raw Elasticsearch response. The result is true when every document indexed successfully (or when there was nothing to index). It is false when the bulk request completed (HTTP 200) but one or more individual documents failed to index; in that case one Koha::Object::Message of type 'error' is recorded per failed document, with the failing record_id and the raw Elasticsearch error in the payload. Whole-operation failures (e.g. Elasticsearch unreachable) keep throwing Koha::Exceptions::Elasticsearch::BadResponse, so the NoNodes recovery path from bug 42669 is preserved. Exceptions mean 'could not run'; a false result means 'ran, but some documents failed'. es_indexer_daemon.pl now inspects the returned Boolean: on a partial failure it logs each failing record and marks the batch 'failed' instead of 'finished', so affected records are no longer silently reported as indexed. Test plan: 1. prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t 2. All subtests pass, including the new 'update_index() return value tests' covering partial failure, full success, empty body and the thrown BadResponse exception. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206319 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206319&action=edit Bug 43569: Phase 2 - harden rebuild_elasticsearch.pl against silent failures Building on the update_index() Koha::Result::Boolean contract from Phase 1, the reindex script no longer reports success when records fail to index. - _handle_response() now consumes the Koha::Result::Boolean, counts the per-document failures, and surfaces a summary regardless of verbosity so cronjobs notice (previously the error line was suppressed at the default verbosity used by cron). - Both the buffered commits and the final flush go through _commit_chunk(), so a whole-operation failure (e.g. Elasticsearch becoming unreachable) on the last chunk is handled like any other chunk instead of dying mid-run under 'use autodie'. - The script now exits non-zero when any record failed to index, giving cron wrappers a way to detect trouble. - In --processes mode, child exit status is aggregated by the parent (wait() return status is now checked), so a failed slice can no longer be hidden behind a clean parent slice. - The final line reports processed vs indexed vs failed counts instead of overstating success. Test plan: 1. koha-shell kohadev -c 'perl misc/search_tools/rebuild_elasticsearch.pl -b' Reindex succeeds and exits 0 (echo $?). 2. Make Elasticsearch unreachable and rerun: the script reports the failure visibly and exits non-zero. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206320 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206320&action=edit Bug 43569: (follow-up) Add POD for _get_record The QA tool flags pod_coverage for _get_record now that this file is touched. Add a short description for the private helper. No functional change. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi all. Submitted early to get your opinion on the implementation details. I believe this is the way to go as I tackled the problem at its roots in `update_index`. I still have doubts about displaying progress (should have a progress callback as we do with imports? etc). And I preferred to delay the logging normalization, but we could just do it here too. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206318|0 |1 is obsolete| | Attachment #206319|0 |1 is obsolete| | Attachment #206320|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206533 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206533&action=edit Bug 43569: Make update_index report per-document failures This patch makes Koha::SearchEngine::Elasticsearch::Indexer::update_index return a Koha::Result::Boolean instead of the raw Elasticsearch response, so callers can tell when individual documents failed to index even though the bulk request itself succeeded. The result is true when every document indexed successfully (or when there was nothing to index). It is false when the bulk request completed (HTTP 200) but one or more individual documents failed to index; in that case one Koha::Object::Message of type 'error' is recorded per failed document, with the failing record_id and the raw Elasticsearch error in the payload. Changes: - update_index() returns a Koha::Result::Boolean carrying per-document error messages instead of the raw Elasticsearch response - Whole-operation failures (e.g. Elasticsearch unreachable) keep throwing Koha::Exceptions::Elasticsearch::BadResponse, preserving the NoNodes recovery path from bug 42669. An exception means 'could not run'; a false result means 'ran, but some documents failed' - misc/workers/es_indexer_daemon.pl now inspects the returned Boolean: on a partial failure it logs each failing record and marks the batch 'failed' instead of 'finished', so affected records are no longer silently reported as indexed Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t => SUCCESS: All subtests pass, including the new 'update_index() return value tests' covering partial failure, full success, empty body and the thrown BadResponse exception 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206534 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206534&action=edit Bug 43569: Harden rebuild_elasticsearch.pl against silent failures This patch builds on the update_index() Koha::Result::Boolean contract from the previous patch so that misc/search_tools/rebuild_elasticsearch.pl no longer reports success when records fail to index. Changes: - _handle_response() now consumes the Koha::Result::Boolean, counts the per-document failures, and surfaces a summary regardless of verbosity so cronjobs notice (previously the error line was suppressed at the default verbosity used by cron) - Both the buffered commits and the final flush go through _commit_chunk(), so a whole-operation failure (e.g. Elasticsearch becoming unreachable) on the last chunk is handled like any other chunk instead of dying mid-run under 'use autodie' - The script now exits non-zero when any record failed to index, giving cron wrappers a way to detect trouble - In --processes mode, child exit status is aggregated by the parent (wait() return status is now checked), so a failed slice can no longer be hidden behind a clean parent slice - The final line reports processed vs indexed vs failed counts instead of overstating success Test plan: 1. Apply this patch 2. Reindex a healthy Elasticsearch: $ ktd --shell k$ perl misc/search_tools/rebuild_elasticsearch.pl -b k$ echo $? => SUCCESS: Reindex succeeds and exits 0 3. Make Elasticsearch unreachable and rerun step 2 => SUCCESS: The script reports the failure visibly and exits non-zero 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 206535 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206535&action=edit Bug 43569: (follow-up) Add POD for _get_record The QA tool flags pod_coverage for _get_record now that Koha/SearchEngine/Elasticsearch/Indexer.pm is touched. This patch adds a short POD description for the private helper. No functional changes - documentation only. Test plan: 1. Apply this patch 2. Run the QA tool on the branch: $ ktd --shell k$ /kohadevbox/qa-test-tools/koha-qa.pl -c 3 -v 2 => SUCCESS: No pod_coverage failure for _get_record 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206533|0 |1 is obsolete| | Attachment #206534|0 |1 is obsolete| | Attachment #206535|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 207065 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207065&action=edit Bug 43569: Make update_index report per-document failures This patch makes Koha::SearchEngine::Elasticsearch::Indexer::update_index return a Koha::Result::Boolean instead of the raw Elasticsearch response, so callers can tell when individual documents failed to index even though the bulk request itself succeeded. The result is true when every document indexed successfully (or when there was nothing to index). It is false when the bulk request completed (HTTP 200) but one or more individual documents failed to index; in that case one Koha::Object::Message of type 'error' is recorded per failed document, with the failing record_id and the raw Elasticsearch error in the payload. Changes: - update_index() returns a Koha::Result::Boolean carrying per-document error messages instead of the raw Elasticsearch response - Whole-operation failures (e.g. Elasticsearch unreachable) keep throwing Koha::Exceptions::Elasticsearch::BadResponse, preserving the NoNodes recovery path from bug 42669. An exception means 'could not run'; a false result means 'ran, but some documents failed' - misc/workers/es_indexer_daemon.pl now inspects the returned Boolean: on a partial failure it logs each failing record and marks the batch 'failed' instead of 'finished', so affected records are no longer silently reported as indexed Test plan: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t => SUCCESS: All subtests pass, including the new 'update_index() return value tests' covering partial failure, full success, empty body and the thrown BadResponse exception 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 207066 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207066&action=edit Bug 43569: Harden rebuild_elasticsearch.pl against silent failures This patch builds on the update_index() Koha::Result::Boolean contract from the previous patch so that misc/search_tools/rebuild_elasticsearch.pl no longer reports success when records fail to index. Changes: - _handle_response() now consumes the Koha::Result::Boolean, counts the per-document failures, and surfaces a summary regardless of verbosity so cronjobs notice (previously the error line was suppressed at the default verbosity used by cron) - Both the buffered commits and the final flush go through _commit_chunk(), so a whole-operation failure (e.g. Elasticsearch becoming unreachable) on the last chunk is handled like any other chunk instead of dying mid-run under 'use autodie' - The script now exits non-zero when any record failed to index, giving cron wrappers a way to detect trouble - In --processes mode, child exit status is aggregated by the parent (wait() return status is now checked), so a failed slice can no longer be hidden behind a clean parent slice - The final line reports processed vs indexed vs failed counts instead of overstating success Test plan: 1. Apply this patch 2. Reindex a healthy Elasticsearch: $ ktd --shell k$ perl misc/search_tools/rebuild_elasticsearch.pl -b k$ echo $? => SUCCESS: Reindex succeeds and exits 0 3. Make Elasticsearch unreachable and rerun step 2 => SUCCESS: The script reports the failure visibly and exits non-zero 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 --- Comment #10 from David Nind <david@davidnind.com> --- Created attachment 207067 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207067&action=edit Bug 43569: (follow-up) Add POD for _get_record The QA tool flags pod_coverage for _get_record now that Koha/SearchEngine/Elasticsearch/Indexer.pm is touched. This patch adds a short POD description for the private helper. No functional changes - documentation only. Test plan: 1. Apply this patch 2. Run the QA tool on the branch: $ ktd --shell k$ /kohadevbox/qa-test-tools/koha-qa.pl -c 3 -v 2 => SUCCESS: No pod_coverage failure for _get_record 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43569 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org