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: [email protected]
Reporter: [email protected]
QA Contact: [email protected]
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.
_______________________________________________
Koha-bugs mailing list -- [email protected]
To unsubscribe send an email to [email protected]
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
