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.