Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review: partial-commit / partial-write reporting on batch & time-series endpoints

Thanks for tackling this - the silent-data-loss problem is real and the direction (surface counts plus a partialCommit / partial-write signal so clients can reconcile rather than blindly retry) is the right one. Documenting the non-atomic contract in the Javadoc and matching InfluxDB partial-write semantics are both nice touches, and both new paths have regression tests. A few concerns, roughly by severity.

1. PostBatchHandler broad catch(Exception) collapses the base handler status mapping (high)

The new try/catch in execute() maps everything to IllegalArgumentException -> 400, else 500. Previously the exception propagated to AbstractServerHttpHandler, which has a carefully-built mapping this now bypasses:

• DuplicatedKeyException -> 409 ("do not retry", issue 4350)
• TransactionCommittedRemotelyException -> 409 ("do not retry", 5064/5075)
• NeedRetryException -> 503 (retryable)
• ServerSecurityException / SecurityException -> 403
• RecordNotFoundException -> 404

For the batch endpoint these are not hypothetical: GraphBatch.createVertices commits internally and can throw DuplicatedKeyException (a batch vertex hitting a unique index) or NeedRetryException (Raft hiccup). Both now return 500 instead of 409/503. The 409 arms exist specifically because a 5xx invites HTTP clients and load balancers to retry, and a retry of an already-committed chunk duplicates data - the exact hazard this PR is trying to eliminate. So collapsing to 500 here works against the PR own goal. Consider re-throwing the exceptions the base handler already maps (or letting them propagate) and only wrapping the cases where you want to attach the partial-commit counts. At minimum, the "do not retry" cases should not degrade to a retry-inviting 500.

2. Time-series dropped count mixes units with written (medium)

dropped = unknownTypes.size() + nonTimeSeriesTypes.size() counts distinct dropped measurement NAMES (both are Set), while written = inserted counts SAMPLES. If 100 samples for one unknown measurement arrive, the client sees dropped:1 next to written:N and will reasonably read it as "1 sample dropped". The new test only sends one sample per measurement, so the mismatch is invisible. Either count actual dropped samples (increment a counter in the continue branches) or rename the field to droppedMeasurements so the unit is unambiguous.

3. Reported batch counts are optimistic vs what is durable (low)

verticesCreated / edgesCreated track records submitted to the batch, but GraphBatch only commits every commitEvery (default 50000). On mid-stream failure the records since the last commit boundary are rolled back, so the reported count can overstate what actually survived on disk. The test uses commitEvery=1 so it lines up exactly, but with defaults a client reconciling against these numbers could over- or under-delete. Worth a Javadoc note that the counts are "attempted" (upper bound).

4. 500 path drops the stack trace (low)

The catch logs only Level.WARNING with e.getMessage(). For the genuine-server-error (500) case the previous path logged via the base handler with the full throwable. Losing the stack trace makes diagnosing real ingest failures harder - consider logging the throwable (with stack) when the status is 500.

Minor / nits

• partialCommit is false when the very first chunk fails before any commit - a useful "nothing durable yet" signal, but given point 3 it is not a hard guarantee of zero writes; make that clear to clients.
• The openWriteConnection test refactor is clean; the added contentEncoding param is currently only exercised as null - fine as prep, just flagging it is unused by the new test.

Overall a solid, well-tested fix. The main items I would want addressed before merge are point 1 (do not let 409/503 outcomes degrade to a retry-inviting 500 on the batch endpoint) and point 2 (the dropped unit mismatch).

Code Review - PR #5167

Thanks for this fix - it closes a genuine data-loss-visibility gap, and the change is careful and well-reasoned. The inline documentation explaining why only IllegalArgumentException is enriched (and why not to downgrade non-retryable outcomes to a 500) is excellent, exactly the kind of context that prevents a future well-meaning "simplification" from reintroducing the bug. I verified a few things against the surrounding code; findings below, ordered by importance.

Should fix

1. Stray </content> tag in the doc file. docs/5036-batch-timeseries-lossy-error-reporting.md ends with a literal </content> line - an artifact from generation that leaked into the committed file. Please drop that last line.

Worth considering

2. Partial-commit error body diverges from the standard error envelope. Elsewhere the server emits errors via AbstractServerHttpHandler.sendErrorResponse, which produces { error, detail, exception, exceptionArgs, requestId? }. The new batch 400 body is { error, verticesCreated, edgesCreated, partialCommit } - it omits exception and detail/requestId. Functionally fine, but a client or log pipeline that keys off exception (class name) or the correlation id won't find them on this specific 400. Adding partialCommit/counts onto the existing envelope (rather than a bespoke shape) would keep the contract uniform. Not blocking - just flagging the inconsistency.

3. Non-IllegalArgumentException mid-stream failures still hide their partial commit. The catch is scoped to IllegalArgumentException (correctly - NumberFormatException from a malformed RID is a subclass, so those are covered too). But a DuplicatedKeyException (409) or a flush failure at batch.close() also leaves earlier commitEvery chunks durable, and the client gets no counts for those. The Javadoc documents this deliberate choice and the rationale is sound (don't invite a duplicate-causing retry), so I'd accept it as-is - but it means issue 5036's underlying "hidden partial commit" concern is only fully solved for the 400 path. Worth a one-line note that the 409/5xx paths remain best-effort by design.

Nits

4. CSV path isn't exercised for partial commit. partialCommitErrorReportsPersistedCounts covers JSONL only. The handler logic is format-agnostic (the counters live in execute, not the stream), so this is low-risk, but a CSV variant would lock in the contract for both BatchRecordStream implementations.

Things I checked and liked

• Status-code continuity verified: IllegalArgumentException was already mapped to 400 by AbstractServerHttpHandler, so this only enriches the body - no status regression. The all-dropped time-series case still returns 400 (dropped > 0 subsumes the old inserted == 0 guard), and clean ingests still return 204.
• Time-series counting is consistent: dropped = samples.size() - inserted matches the skip sets since every parsed sample is either inserted or added to unknownTypes/nonTimeSeriesTypes, and the inserted rows are committed atomically in one transaction before the partial-write signal is built.
• The "counts are an upper bound / at most this many" caveat in the batch Javadoc is genuinely accurate: batch.close() runs during try-with-resources unwinding and its flush() can itself fail (captured as a suppressed exception), so a buffered-edge count may not all be durable. Good that the doc is conservative rather than over-promising.
• Existing time-series tests assert only on status code, so the message-wording/casing change (partial write: ...) doesn't break them. Test helper refactor (openWriteConnection) is clean.

Overall: solid, well-tested change. Just the stray </content> line needs removing before merge; the rest are optional.

Code Review - PR #5167

Solid, well-scoped fix. The core behavior change is correct: both endpoints previously reported success (or a bare error) while data was partially written, and this makes the partial state explicit and reconcilable. The Javadoc/inline comments are unusually thorough and the reasoning for only enriching the IllegalArgumentException/400 path (leaving engine/cluster exceptions to AbstractServerHttpHandler's status mapping) is sound. Nice regression tests for both JSONL and CSV stream formats.

I verified the logic against the surrounding code. A few points, none blocking:

1. Error-envelope inconsistency with AbstractServerHttpHandler (worth a look)

The base handler's buildErrorBody follows a deliberate contract: error holds a generic category string (e.g. "Cannot execute command"), while the free-form exception message goes into detail, which is concealed in production (verbose == false) to avoid leaking file paths / engine internals. It also emits requestId (correlation id) for log cross-referencing.

The new batch 400 body instead puts the raw e.getMessage() directly into error:

error.put("error", message);          // raw exception message, always visible
error.put("exception", e.getClass().getName());

So the batch path bypasses the production concealment and omits requestId. For these batch IllegalArgumentExceptions the messages are client-facing ("Unknown temporary ID 'x' at line N"), so the leak risk is low - but it's an intentional divergence from the documented wire contract. Consider routing through the same envelope builder, or at least omitting requestId on purpose. Same note applies to the time-series handler (error = full message, no requestId).

2. Time-series 204 -> 400 is a client-visible behavior change

Returning 400 on partial write matches InfluxDB semantics and is the right call, but it changes behavior for existing ingest clients (Telegraf's outputs.influxdb, custom line-protocol writers) that today get 204 when at least one sample lands. Telegraf treats 400 as non-retryable and drops the batch, which is the intended InfluxDB behavior - but this should be called out in the release notes/CHANGELOG so operators are not surprised.

3. Broad IllegalArgumentException catch in the batch handler

The catch also swallows NumberFormatException (subclass) from malformed RID parsing in resolveRef (Integer.parseInt/Long.parseLong). Those still correctly map to 400, but the client sees a raw "For input string: ..." in error rather than the handler's clearer "Malformed RID ..." message. Minor - the explicit IllegalArgumentException("Malformed RID ...") you throw for the missing-colon case reads much better; the parse failures fall through with the JDK message.

4. catch runs after try-with-resources closes batch

Because batch.close() (flush edges + connect) runs during resource unwinding before the catch, an IllegalArgumentException thrown from close() on an otherwise-clean stream would be reported with fully-counted verticesCreated/edgesCreated and partialCommit=true. This is consistent with the "upper bound / best-effort" contract you documented, so it's acceptable - just noting the counts are attempted totals, not a durability guarantee, exactly as the Javadoc says.

5. dropped = samples.size() - inserted is correct

Confirmed every parsed sample is either inserted or continued into unknownTypes/nonTimeSeriesTypes, so the sample-count arithmetic holds and the cycle-1 fix (count samples, not distinct type names) is right.

6. Docs nit

docs/5036-batch-timeseries-lossy-error-reporting.md includes an internal "Review cycles" log referencing "claude + gemini" items and a Cycle 2 (<next>) placeholder. That reads like PR-thread scratch notes rather than durable design documentation - consider trimming it before merge so the committed doc stays about the behavior, not the review process.

Tests

Coverage is good: mid-stream partial-commit for both JSONL and CSV, mixed known/unknown time-series ingest with a persistence assertion, and the existing full-success/all-dropped paths are preserved. One suggestion: add a batch case asserting partialCommit=false when the first record fails (0 committed) to lock that boundary - unknownTempIdReturnsError exercises it but only checks the status code, not the new fields.

Overall this is a clean improvement to data-loss visibility. The envelope inconsistency (#1) is the only item I'd flag for a maintainer decision before merge; the rest are nits.

Code Review - PR #5167

Thanks for tackling this - the silent partial-write data loss is a real correctness/observability gap, and the fix is well-scoped. The Javadoc and inline comments are genuinely excellent: they document the non-atomic contract, the rationale for leaving non-retryable engine/cluster exceptions to propagate, and the "upper bound" caveat on the counts. Tests cover the important cases (JSONL, CSV, first-record-fails, mixed TS ingest). A few observations below, mostly around consistency and one behavioral note.

Design is sound (verified)

I traced GraphBatch to confirm the counts are trustworthy: createVertices commits each call immediately via createVerticesWithRetry (line 399), independent of commitEvery, and close() flushes buffered edges on the exception-unwind path (try-with-resources runs close() before the catch). So verticesCreated/edgesCreated genuinely reflect durable records and partialCommit = counts > 0 is accurate, not merely an upper bound. Good defensive framing in the Javadoc regardless.

1. Error body diverges from the standard error envelope (medium)

PostBatchHandler now builds the 400 body by hand instead of routing through AbstractServerHttpHandler.buildErrorBody. Two side effects worth weighing:

• Production concealment is bypassed. buildErrorBody deliberately emits a fixed generic error string and pushes the raw exception message/cause chain into detail, which is hidden outside dev/test mode (to avoid leaking engine internals to a probing client). Here e.getMessage() is placed directly into error and always emitted. For batch-parsing IllegalArgumentExceptions the messages just echo client input (line numbers, temp ids, malformed RIDs), so the risk is low - but any IAE that bubbles up from deeper engine code would now surface its raw message in production. Worth a conscious decision.
• requestId correlation id is dropped. The standard envelope includes the REQUEST_ID_HEADER correlation id so operators can cross-reference a client-facing 400 with the server log. This custom body omits it. Consider adding requestId (and/or building on top of the standard envelope) for consistency with every other endpoint.

2. InfluxDB 204 -> 400 is a real behavioral change for Telegraf-style clients (informational)

You call this out in the PR body and the doc file, which is the right thing to do. Just amplifying: any deployment that continuously ships a mix where one measurement type simply hasn't been created yet will now get a 400 on every write batch instead of 204, and Telegraf treats 400 as non-retryable (drops the buffered batch). That matches InfluxDB's partial-write semantics and is defensible, but it deserves a prominent CHANGELOG/release-note entry (the doc mentions this - please make sure it lands in the actual release notes, not just docs/5036-*.md).

3. Broad catch (IllegalArgumentException) (low)

This catches any IAE thrown anywhere in the streaming block, including one that might originate from engine internals rather than client input, and reports it as a 400 client error with partialCommit. IAE almost always means bad input here, so this is acceptable - but a genuine engine bug that manifests as an IAE would now be masked as a client 400 rather than a 500 with a logged stack trace. Minor; noting for awareness.

Minor / nits

• dropped = samples.size() - inserted in the TS handler is correct given the insert loop either inserts or continues into a drop set, and any real insert failure rolls back the whole tx and rethrows before this line is reached. The comment explaining this invariant is helpful.
• The message-casing changes ("Unknown timeseries type(s)" -> "unknown timeseries type(s)" prefixed by "partial write: ") are a fine cleanup; just confirm no downstream client/test elsewhere string-matches the old capitalization.

Test coverage

Good breadth - partialCommitFalseWhenFirstRecordFails and the CSV variant are nice additions that lock in the boundary behavior. Two optional gaps you might consider:

• A batch that fails mid-stream on an edge after edges were already committed (edgesCreated > 0) - all current batch tests land edgesCreated=0.
• The all-dropped TS case (written=0) to pin that it still returns 400 with the same new payload shape (the PR says it's preserved, but there's no assertion on the new written/dropped fields for that path).

Overall a solid, well-documented fix. The main thing I'd want resolved before merge is the error-body divergence in item 1 (at minimum restoring requestId, and a deliberate call on the production message exposure).

Reviewed against CLAUDE.md conventions. Automated review - please validate.

Review: PR #5167 - partial-commit / partial-write reporting

Thanks for tackling #5036. This is a well-scoped, well-documented change that closes a real data-loss-visibility gap on two bulk-write endpoints. The Javadoc and inline comments are unusually thorough, the "attempted vs. durable" semantics are stated honestly rather than oversold, and the new tests cover the important axes (mixed drop, all-dropped, first-record-fails, JSONL and CSV paths, plus a persistence assertion on the time-series side). Nice work.

A few observations, mostly minor.

Correctness

• dropped = samples.size() - inserted is sound given the control flow: it is only reached on the commit-success path, where every parsed sample is either inserted++ or added to one of the two drop sets (a mid-loop engine.appendSamples failure rolls back and re-throws before this line). Counting samples while the sets name distinct measurements is correctly documented.

• Batch counters reflect "attempted", not "durable". verticesCreated is incremented at flush time, and on the exception path the try-with-resources batch.close() still runs (flushing/committing pending work) while a partial commitEvery chunk may roll back - so the reported number can be either slightly high or slightly low versus what actually landed. The Javadoc calls this out as "at most this many", which is the honest framing; just be aware the count is genuinely approximate and a reconciling client must still scan.

Scope limitation worth surfacing

The core goal - "let the client detect a partial write instead of blindly retrying" - is only met for the IllegalArgumentException (400) path. Engine/cluster failures (NeedRetryException -> 503, DuplicatedKeyException -> 409, commit failures -> 500, etc.) propagate to AbstractServerHttpHandler and the client gets no verticesCreated/partialCommit info at all - yet those are exactly the cases named in the issue (dropped connection, mid-stream commit failure) that leave committed chunks behind. The Javadoc labels this "best-effort", a reasonable v1 boundary, but the most dangerous scenario (transient 503 mid-load -> client retries) is still silent. Consider a follow-up that attaches the counts to those responses too.

Security / consistency nuance

The standard buildErrorBody deliberately conceals the free-form cause chain (detail) in production to avoid leaking engine internals/file paths, emitting only the bounded error/exception/requestId fields. The bespoke batch body instead places the raw exception message into error unconditionally, in all modes. The comment argues batch IAE messages only echo client input - true for this handler's own resolveRef/processEdge throws, but an IllegalArgumentException can also originate deep in the engine (schema/type validation, conversion) with a message that is less obviously client-safe. Low risk, but since IAE is caught broadly it is not a hard guarantee.

Minor

• The broad catch (IllegalArgumentException) will also 400-classify any non-client IAE bubbling up from GraphBatch/engine internals. Acceptable given IAE is overwhelmingly client-input here; flagging the assumption.
• Test-plan checkboxes in the PR body are left unchecked ([ ]) though the text says 16/16 and 7/7 pass - worth ticking so reviewers know they ran.
• The time-series 204 -> 400 change on partial write is a genuine client-visible contract change (Telegraf treats 400 as non-retryable). It is documented in the doc file and PR body - please make sure it also lands in the actual CHANGELOG/release notes, not just docs/5036-*.md.

Overall: a solid, defensible change with honest documentation. The main thing I would push on is whether the non-IAE failure paths (503/409/500) - the ones most likely to actually strand committed data - deserve the same treatment in a follow-up.

Codecov Report

❌ Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.99%. Comparing base (96aca46) to head (e0f6828).
⚠️ Report is 6 commits behind head on main.

@@             Coverage Diff              @@
##               main    #5167      +/-   ##
============================================
+ Coverage     65.97%   65.99%   +0.02%     
+ Complexity      916      915       -1     
============================================
  Files          1693     1693              
  Lines        136805   136837      +32     
  Branches      29253    29259       +6     
============================================
+ Hits          90251    90308      +57     
+ Misses        34308    34284      -24     
+ Partials      12246    12245       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.