Return false from bulkUpsert when write result is incomplete by Bhuvan506 · Pull Request #322 · hypertrace/document-store

Bhuvan506 · 2026-08-24T11:27:09Z

Summary

Mongo bulkUpsert: return false when the write is unacknowledged or matchedCount + upserts.size() != requested (unordered bulk write can acknowledge without accounting for every doc).
Postgres / FlatPostgres bulkUpsert: on BatchUpdateException, log partial updateCounts and return false (success-path batch length checks removed — they always passed when no exception was thrown).
bulkUpsertAndReturnOlderDocuments: fail with IOException on incomplete/failed upsert and close the Mongo cursor / Postgres ResultSet so resources are not leaked.
Hard failures still map to false / IOException as before.

Context

Investigated while looking at attribute-service create returning success with missing attribute_metadata docs. Attribute-service is also adding client-side post-write verification for evidence; this PR tightens the shared library contract.

Test plan

Unit: MongoCollectionTest.BulkUpsert
Integration: MongoPostgresWriteConsistencyTest.BulkUpsertConsistencyTest (complete batch + older-documents consistency) — needs Docker/CI

Mongo and Postgres bulkUpsert previously returned true whenever no exception was thrown, ignoring BulkWriteResult / JDBC batch counts. Callers (e.g. attribute-service) then treated partial writes as success. Validate that every requested document is accounted for before reporting success; keep throwing/returning false on hard failures. Co-authored-by: Cursor <cursoragent@cursor.com>

codecov · 2026-08-24T11:31:06Z

Codecov Report

❌ Patch coverage is 49.15254% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.81%. Comparing base (330cbc2) to head (d81d04b).

Files with missing lines	Patch %	Lines
...race/core/documentstore/mongo/MongoCollection.java	48.57%	17 Missing and 1 partial ⚠️
...ore/documentstore/postgres/PostgresCollection.java	42.85%	11 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #322      +/-   ##
============================================
- Coverage     81.06%   80.81%   -0.25%     
- Complexity     1617     1631      +14     
============================================
  Files           243      243              
  Lines          7656     7708      +52     
  Branches        755      763       +8     
============================================
+ Hits           6206     6229      +23     
- Misses          960      987      +27     
- Partials        490      492       +2

Flag	Coverage Δ
integration	`80.81% <49.15%> (-0.25%)`	⬇️
unit	`57.25% <20.33%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-08-24T11:31:47Z

Test Results

126 files +1 126 suites +1 38s ⏱️ -2s
864 tests +9 863 ✅ +9 1 💤 ±0 0 ❌ ±0
1 201 runs +9 1 200 ✅ +9 1 💤 ±0 0 ❌ ±0

Results for commit d81d04b. ± Comparison against base commit 330cbc2.

♻️ This comment has been updated with latest results.

Address review feedback: keep the helper with the Postgres hierarchy (FlatPostgresCollection already extends it) instead of a commons util. Co-authored-by: Cursor <cursoragent@cursor.com>

suddendust · 2026-08-27T05:16:07Z

+  class BatchFullySuccessful {
+
+    @Test
+    void allPositive_returnsTrue() {


Lets stick to camelCase for consistency.

Removed those nested tests (Postgres success-path batch-length check was dropped per your later comment). New IT method names are camelCase.

suddendust · 2026-08-27T05:16:37Z

+            result.wasAcknowledged() ? result.getUpserts().size() : -1,
+            result.wasAcknowledged(),
+            result);
+        throw new IOException("Incomplete bulk upsert.");


This might now break existing clients.

Agreed it tightens the contract: callers that previously treated incomplete Mongo writes as success will now get false / IOException. That is intentional for the gap we care about. Attribute-service is also adding a client-side post-write verify + evidence logs; we can stage rollout if needed.

suddendust · 2026-08-28T05:19:55Z

+            result.wasAcknowledged() ? result.getUpserts().size() : -1,
+            result.wasAcknowledged(),
+            result);
+        throw new IOException("Incomplete bulk upsert.");


We should close the cursor here or it'll be a resource leak.

Fixed — close the Mongo cursor before throwing on incomplete upsert (and on other failure paths before the iterator takes ownership).

suddendust · 2026-08-28T05:22:04Z

+            "Incomplete bulk upsert for documents. requested={}, updateCounts={}",
+            documents.size(),
+            Arrays.toString(updateCounts));
+        throw new IOException("Incomplete bulk upsert.");


Close the RS here to prevent leak?

Fixed — close the ResultSet in a finally when we do not hand it to PostgresResultIterator.

suddendust · 2026-08-28T05:44:02Z

+   */
+  @VisibleForTesting
+  static boolean isBatchFullySuccessful(final int[] updateCounts, final int expectedSize) {
+    if (updateCounts == null || updateCounts.length != expectedSize) {


This method will always return true due to the way we're building the batch. Because we add 1 doc per batch, updateCounts.length == expectedSize will always hold true. Also, if a single doc/batch failed upsert, it'll throw BatchUpdateException.

Rather, smth like this should help:

} catch (BatchUpdateException e) { int[] partial = e.getUpdateCounts(); LOGGER.error( "BatchUpdateException bulk inserting documents. requested={}, updateCounts={}", documents.size(), Arrays.toString(partial), e); return false; // partial application: some entries succeeded, some EXECUTE_FAILED }

Can you check this?

Checked — agree. Dropped the success-path isBatchFullySuccessful check. BatchUpdateException now logs requested + e.getUpdateCounts() and returns false / fails bulkUpsertAndReturnOlderDocuments.

suddendust · 2026-08-28T05:48:46Z

Lets add some integration tests here to validate the behaviour consistency b/w Mongo and PG.

Close Mongo cursors / Postgres ResultSets on bulkUpsertAndReturnOlderDocuments failure paths. For Postgres, rely on BatchUpdateException (log partial updateCounts) instead of a success-path batch length check that always passed. Add Mongo/PG bulkUpsert consistency integration tests. Co-authored-by: Cursor <cursoragent@cursor.com>

Bhuvan506 · 2026-08-28T06:47:50Z

Added Mongo/PG consistency ITs in MongoPostgresWriteConsistencyTest.BulkUpsertConsistencyTest: complete-batch success + readability, and bulkUpsertAndReturnOlderDocuments behaviour on both stores.

Bhuvan506 requested review from avinashkolluru, kotharironak, puneet-traceable, skjindal93, suddendust and suresh-prakash as code owners August 24, 2026 11:27

Bhuvan506 requested a review from saxenakshitiz August 24, 2026 14:12

saxenakshitiz reviewed Aug 26, 2026

View reviewed changes

Comment thread document-store/src/main/java/org/hypertrace/core/documentstore/commons/BatchWriteUtils.java Outdated

Move JDBC batch success check onto PostgresCollection

a1eae2f

Address review feedback: keep the helper with the Postgres hierarchy (FlatPostgresCollection already extends it) instead of a commons util. Co-authored-by: Cursor <cursoragent@cursor.com>

saxenakshitiz previously approved these changes Aug 27, 2026

View reviewed changes

suddendust reviewed Aug 27, 2026

View reviewed changes

suddendust reviewed Aug 28, 2026

View reviewed changes

Bhuvan506 dismissed saxenakshitiz’s stale review via d81d04b August 28, 2026 06:47

Uh oh!

Conversation

Bhuvan506 commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Test plan

Uh oh!

codecov Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suddendust commented Aug 28, 2026

Uh oh!

Bhuvan506 commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bhuvan506 commented Aug 24, 2026 •

edited

Loading

codecov Bot commented Aug 24, 2026 •

edited

Loading

github-actions Bot commented Aug 24, 2026 •

edited

Loading