docs(spec): one document that cannot be serialized fails its whole reindex batch (#37269)

[fabrizzio-dotCMS]

Spec PR (1 of 2) for #37269. It carries spec.md only; the implementation PR follows once this is approved.

Problem

When one document in a reindex group can't be serialized (e.g. a field over Jackson's 20,000,000-character read limit), the whole engine request is abandoned and every contentlet in the group is recorded as failed with that one document's message. On a real dataset, 10 oversized documents caused 380 failures, 363 of them healthy contentlets under 100 KB.

The trigger fails on Elasticsearch because the 7.10.2 client re-parses every document while building the bulk body (RequestConverters.bulk()). OpenSearch parses each document as it is added, so there only the bad document fails. But OpenSearch's flush() still discards the whole group on any other client-side error, and Phase 3 inherits that.

What the spec asks for

• A document that can't be serialized fails on its own, and the rest of its group is indexed. This applies with ES as primary (Phase 0) and OS as primary (Phase 3).
• The failure record names the offending contentlet and the value's real size, not the parser's buffer size.
• In Phase 1, a failure on the shadow engine alone doesn't mark as failed a contentlet the primary indexed.
• Transport and access failures (engine down, timeout, auth) keep today's behaviour: the whole group is failed and retried.

Out of scope

• A configurable or JVM-wide raised parser limit.
• A record of OS shadow failures: moved to Reindex failures in dual-write phases do not report which engine failed #37271.
• Retrying failed records: Failed reindex records are never retried, and clearing them silently abandons unindexed content #37270.
• A save-time guard against data: URIs: separate issue.

Verification

A new integration test (one field over 20M characters plus N healthy contentlets in the same fetched group), registered in a MainSuite and run at Phases 0, 1 and 3. It must fail on current code first.

🤖 Generated with Claude Code

[claude]

Claude finished @fabrizzio-dotCMS's task in 57s —— View job

---

Spec review — #37269 bulk batch discard

This PR carries only specs/37269-bulk-batch-discard/spec.md (+266, no code), so there's nothing to review for runtime bugs/security. I checked the spec for internal consistency and for alignment with the comment thread (AC-005 sibling-document guarantee, the Phase 1 wording correction, and the new AC-007 endpoint).

New Issues

• 🟡 Medium: specs/37269-bulk-batch-discard/spec.md:243 — priority is in AC-007 but missing from the In-Scope field list. AC-007 says each entry carries "…failed attempts, priority, last failure reason…", but the In-Scope description of the same response (lines 177–179) lists "…the failed attempts, the last failure reason…" with no priority. Pick one so the acceptance criterion and the scope agree on the response shape. Fix this →

Observations (non-blocking, no change required)

• spec.md:173 — "12 failed records … 168 MB response" vs. the PR description's "a few failed records" and the Problem Statement's "10 oversized documents" (line 23). Not contradictory (different runs), but a reader may trip on the three different counts. Optional to harmonize or footnote.
• spec.md:185 — "the 20 MB parser limit" is shorthand for the 20,000,000-character StreamReadConstraints limit used precisely everywhere else in the doc. Fine as prose, just noting the unit shift (chars → MB).
• The two items flagged in the thread are correctly reflected: the Phase-1-only shadow wording (lines 79–81, 168–169, 228–230) and the AC-005 sibling-row guarantee that a sibling success must not delete the failure row (lines 109–120, 155–159, 231–235). Root-Cause now reads "Three facts combine" (line 85) consistent with that addition.

The spec is otherwise coherent and the acceptance criteria are testable (Red-first integration test required at Phases 0/1/3, AC-006 transport-vs-content unit test, AC-007 Postman + no-field-values unit test). Only the priority mismatch is worth resolving before the implementation PR locks the response contract.
· 37269-bulk-batch-discard

[dario-daza]

What "fails individually" means when one reindex entry maps to several documents (AC-001)

A reindex entry is per identifier, but it can turn into several index documents. findContentToReindex keys the batch by getIdentToIndex(), and mapEntry → loadVersionInodes loads every working and live inode of that identifier, in every language, through findContentletVersionInfos(identifier). Each one becomes its own document (identifier_lang_variant). The spec should say what the guarantee means in that case:

1. What fails: the document or the identifier? If one language or one version is oversized, are the identifier's other documents still indexed, or does the whole identifier fail? Both are defensible. Today the journal row is per identifier, so failing the whole identifier is the only outcome that leaves a failure record. Either way, the spec should say which one, because AC-001 counts "exactly one failure record" per contentlet.

2. A healthy sibling document must not erase the failure record. BulkProcessorListener.afterBulk(long, List) cuts each result id down to the identifier (result.id().substring(0, sep)) and looks it up in workingRecords. On success, handleSuccess calls deleteReindexEntry. markAsFailed, on the other hand, is just an UPDATE on that same row. So if a sibling document of the failing identifier gets queued and indexed, its success deletes the row that holds the failure.

   A realistic case: a published page whose live version is fine, plus a new working draft with Google Docs HTML pasted in, carrying data: base64 images over the limit. If the check lives at the ES add site (ContentletIndexOperationsES.addIndexOpToProcessor, as the root-cause section suggests), the live document may be queued first, since loadVersionInodes returns a HashMap. Then the working document throws and appendBulkRequestToProcessor marks the entry failed. The live document's bulk success then deletes the journal row. Result: no failure record, the oversized draft never reaches the index, and AC-001 passes or fails depending on map iteration order.

Suggestion: state in the spec that the check runs before anything for the entry is queued. In practice that means inside mapEntry / mapContentletForProcessor, on the Map from toMap(), before writeValueAsString. A throw there already lands in the existing catch in appendBulkRequestToProcessor (ContentletIndexAPIImpl.java:2703), which marks only that entry. So no sibling is queued and the record can't be erased. Then add an acceptance scenario for it: an identifier with a healthy live version and an oversized working version, or with two languages where one is oversized. It should end with one failure record for that identifier and no partial documents in the index.

[fabrizzio-dotCMS]

@dario-daza Good catch — confirmed in the code, and it already applies to the OS path today. Updated the spec (285e4f4):

• The check moves to the mapping step as you suggested: on the final toMap() result (catchall included), before anything of the entry is queued, against StreamReadConstraints.defaults().getMaxStringLength().
• We went with document-level failure: the oversized document is withheld, its healthy siblings are indexed, and the listener must not delete the journal row of an identifier that had a failure in that batch. Failing the whole identifier would drop a healthy live page from the new index on every full reindex.
• Your scenarios are now AC-005 (healthy live + oversized working, and two languages with one oversized), run in both load orders.

[fabrizzio-dotCMS]

Wording correction, no change in behaviour or acceptance criteria:

The spec says OpenSearch write failures are fire-and-forget "in Phases 1 and 2", and scopes the shadow fix to "Phase 1/2". In the reindex path OS is a shadow only in Phase 1 (createBulkProcessor: !isReadEnabled(), since #37276); in Phase 2 its failures already mark the entry. The implementation isolates the shadow add failure in Phase 1 only, which is what AC-004 already states.

I'd like to change those two sentences to "Phase 1". OK to update the spec, or do you prefer it stays as approved?

[fabrizzio-dotCMS]

@dario-daza I went ahead with the wording change in f3f2454: "Phases 1 and 2" → "Phase 1" in the two sentences (the second also now says failures propagate from Phase 2 on). No change to scope or acceptance criteria. The push dismissed your approval — could you re-approve when you get a chance? Implementation PR: #37924.

[fabrizzio-dotCMS]

@dario-daza one more addition before you re-approve, found while testing the fix by hand: GET /api/v1/esindex/failed embeds each failed record's full contentlet, so a few failed records of oversized content made a 168 MB response and the Maintenance "Download Failed Records" button crashed the browser tab. Added to scope (new AC-007): a vendor-neutral GET /api/v1/index/failed with a typed response and no field values, used by the button; the old endpoint keeps its response and is marked deprecated. Commit fa86a6e. Both changes (wording + this) can go in a single re-approval.

[claude]

Automated rollback-safety check (and note: an earlier placeholder/test string briefly appeared here while verifying tool permissions — this comment replaces it).

Verdict: Safe to roll back. This PR's diff (4e4cdcf4...8a296680) adds a single file, specs/37269-bulk-batch-discard/spec.md — a Spec-Kit specification document (PR 1 of the spec/implementation flow). No Java, SQL/runonce migration, Elasticsearch mapping, data model, or REST/GraphQL contract changes are present, so none of the categories in docs/core/ROLLBACK_UNSAFE_CATEGORIES.md apply.

[fabrizzio-dotCMS]

@dario-daza re-validation request — the spec changed in three places since your approval (all in this PR, and the implementation PR #37924 carries the identical spec.md):

1. Wording (f3f2454): the OpenSearch write shadow is Phase 1 only, not "Phases 1 and 2". No behaviour or AC change.
2. New scope, AC-007 (fa86a6e): found while testing by hand — GET /api/v1/esindex/failed embeds each failed record's full contentlet, so a dozen oversized records made a 168 MB response and the Maintenance "Download Failed Records" button crashed the tab. Adds GET /api/v1/index/failed without field values; the old endpoint is kept unchanged and deprecated.
3. AC-007 detail (8a29668): the listing's shape — document limits and retry policy at the root; per record the pending operation, failed attempts, last failure reason and, for a document-limit failure, the violation (limit, field, actual, allowed, unit); explicit nulls.

Could you take another look and re-approve if it reads right?