R1FIX.3 — preserve the HTTP status of thrown client exceptions (#184)

[fupelaqu]

Closes #184. Story R1FIX.3 of the R1 defect-closure epic (Layer 0, elasticsql).

GetApi.getAsync hardcoded statusCode = Some(500) and never set cause, so a missing index surfaced as an undifferentiated server error and every IF EXISTS / 404-keyed branch misfired — including DROP MATERIALIZED VIEW IF EXISTS (companion: extensions#40).

The fix

One overridable seam on the existing ElasticClientHelpers trait — private[client] def statusOf(t: Throwable): Option[Int], with a concrete None default plus the total statusOrServerError used by core's asynchronous flattening sites. It is overridden once per client family, where the ES exception types are already on the classpath. No new trait, no signature churn, and because the default is concrete, NopeClientApi / MockElasticClientApi / ElasticClientDelegator and every third-party client compile untouched.

None keeps meaning "no HTTP status could be determined" — it is not a synonym for 500 and must never be read as "not found". Only the async flattening sites normalise unknown to 500; the synchronous branches still emit None.

⚠️ How to read the tests — this matters, or the suite looks vacuous

The four-ES-line assertion is a contract, not four fixes.

Line	Before the fix	After
es8 / es9	🔴 RED, 2 of 4 — getAsync and getAsyncAs both Some(500) was not equal to Some(404). Their sync get was already green.	9/9
es7 / es6-rest	🟢 GREEN 4/4 before any production change — they never had the bug (executeGetAsync returns a successful Future wrapping an ElasticFailure that already carries the status).	8/8
es6 / jest	🔴 RED, 2 of 4 — and the issue text and the story spec were both wrong about this line. They claimed jest already returned Some(404); measured against live ES 6.8.23 it returned None.	4/4

Red-before-green was captured for real: the testkit trait and its five subclasses were written and run first, with no production code, and the es8 output was Tests: succeeded 2, failed 2.

The es6/jest discovery

Our own JestClientResultHandler.completed wrapped every non-succeeded JestResult in a status-less new Exception(...), which makes executeAsyncJestAction's getResponseCode mapping unreachable on the async path. That is the same defect class as #184 — our code fabricating a throwable from a status-bearing response and discarding the status — so it is fixed here. Blast radius is provably contained: that handler has exactly one consumer repo-wide, and the error message is byte-identical to before. There is still deliberately no statusOf override in JestClientHelpers.

Also fixed (same defect class)

• The four sibling async flattening sites (indexAsync, updateAsync, deleteAsync, insertByQuery + copyInto) now derive the status and keep the exception as ElasticError.cause.
• updateAsync reported operation = Some("deleteAsync") and "Exception while deleting…" — a verbatim copy-paste from DeleteApi that sent anyone debugging a failed UPDATE to the wrong API. ⚠️ Behaviour note for reviewers: ElasticError.operation is part of the public failure payload, so anything keyed on the old (wrong) value changes. Nothing in-repo keys on it; metrics label independently.
• ElasticClientDelegator did not forward copyInto, so on the wrapper the REPL and JDBC driver actually hold (MonitoredElasticClient → MetricsElasticClient → ElasticClientDelegator) the whole derivation was inert. It now forwards copyInto and the seam itself, so no wrapper can ever disagree with the client it wraps.

What the code review changed

Three parallel adversarial layers ran before commit. No BLOCKER; the acceptance audit returned "no BLOCKER or HIGH — satisfies AC 1-8 and every audited constraint". Thirteen patches were applied anyway, four of which are worth your attention:

1. 🔴 The copyInto forward silently removed all COPY INTO metrics. Before it, the wrapper ran core's body and the operation was measured incidentally through the getIndex / bulkWithResult calls that body makes on this. Forwarding routed it past MetricsElasticClient entirely, so COPY INTO would have contributed zero operations to getMetrics and MonitoredElasticClient's alerts would have stopped seeing file-copy traffic — on the default deployment path. Repaired with an explicit measureAsync("copyInto", …). General rule this establishes: adding a delegator forward can un-measure an operation.
2. statusOrServerError was not actually total. NonFatal excludes LinkageError, and NoClassDefFoundError is an observed failure mode in this repo (ES6 client crashes with NoClassDefFoundError: org/apache/logging/log4j/LogManager (log4j fully excluded from the es6 closure) #168) — the es8/es9 case ex: ResponseException is an instanceof against a class a shaded/ProGuarded fat jar can remove. It now absorbs LinkageError too; OutOfMemoryError still propagates.
3. unwrapThrowable recursed forever on a 2-object cause cycle, and the resulting StackOverflowError is fatal — nothing caught it and the Promise hung. Now iterative with a depth cap.
4. The first depth-limit attempt used a trait val and broke binary compatibility. core/test went red instantly with AbstractMethodError: … UnwrapDepthLimit_$eq from three unrelated pre-existing specs: a val in a trait adds an initializer setter to the interface that every pre-compiled implementor must supply. Made method-local. Worth remembering — it is exactly the guarantee this change relies on.

The four Promise sites also now complete before they log, because this PR newly passes the throwable to logger.error, which makes the appender walk the whole cause chain.

Verification

core/test	669/669, 21 suites, 0 aborted
Cross-compile 2.12 + 2.13, headerCheck, scalafmtCheck	17 successes, 0 errors
es8 / es9 / es7 / es6-rest / es6-jest error-status + statusOf specs	9 / 9 / 8 / 8 / 4 — all green
es8java/testOnly *JavaClient* (regression sweep)	270/270, 15 suites, 0 aborted

es6jest/test reports 232 passed, 0 failed, and 3 suites aborted — JestClientCompanionSpec, JestClientPipelineApiSpec, JestBulkApiSpec. Those are pre-existing and environmental, A/B-proven against pristine main @ 2e02e45c: EmbeddedElasticTestKit launches an x86 embedded ES that cannot boot on an arm64 machine (Unrecognized VM option 'UseAVX=2'). They fail in the spec constructor, before any client code runs.

Version line

Bumped build.sbt:23 from 0.20.2 to 0.20.3-SNAPSHOT. This is the first elasticsql PR of the epic, so it owns the line; R1FIX.4 and R1FIX.5 must verify it and change nothing.

🔴 Nothing was published

No publish, no publishLocal, no release.yml dispatch, no v* tag. This story ends at the PR — the release is yours.

Task 10 (the per-client statusOf unit specs) was marked OPTIONAL/PROPOSED in the spec, to be dropped if the Probe stub fought the _: JavaClientCompanion => / _: RestHighLevelClientCompanion => self-types. It did not — all four were written, they satisfy both self-types with only an elasticConfig override, and they are Docker-free (apply() is never called).

🤖 Generated with Claude Code