[Cosmos Spark] Surface empty change feed pages to avoid end-to-end timeout

[tvaron3]

Summary

Spark partition tasks reading change feed (or executing cross-partition queries) against a sparse workload could hit OperationCancelledException ("End-to-end timeout hit when trying to retrieve the next page") at the connector's 65-second per-operation end-to-end timeout.

Root cause: with the default emptyPagesAllowed=false, ParallelDocumentQueryExecutionContext and ChangeFeedFetcher swallow empty / 304 pages internally — a single producer-side nextPage() call can keep draining many sub-feedRanges before emitting one non-empty page. For sparse cross-partition workloads the cumulative wall time blows the per-operation timeout.

Fix

1. Spark ItemsPartitionReader (query path) calls setAllowEmptyPages(true) on CosmosQueryRequestOptions so the SDK's existing emptyPagesAllowed plumbing applies — each physical page surfaces independently and is bound by the per-page timeout window rather than the per-operation one.

2. New internal-only emptyPagesAllowed flag on CosmosChangeFeedRequestOptionsImpl (default false — behavior unchanged for all other callers). Plumbed through Paginator.getChangeFeedQueryResultAsObservable into ChangeFeedFetcher.nextPageInternal. When the flag is true, both 304/noChanges branches return Mono.just(r) so empty pages bubble up to the iterator. The flag is reached via the existing accessor.getImpl(options).setEmptyPagesAllowed(...) escape hatch — no new bridge accessor wrappers are added (see commit 34d3da4 for the rationale on keeping the accessor surface small for internal-only flags).

3. ChangeFeedFetcher.isFullyDrained correctness fix. The original cleanup (consult only continuation.isDone()) regressed every non-Spark caller because FeedRangeCompositeContinuationImpl.isDone() returns compositeContinuationTokens.size() == 0, which is permanently false for incremental change feed (moveToNextToken() rotates the deque, never shrinks it). The noChanges → true short-circuit is now gated on !emptyPagesAllowed, preserving the original termination signal for every default-mode caller while still letting Spark surface empty pages.

4. Defense-in-depth NO_RETRY arm. With emptyPagesAllowed=true and a streaming change feed (no endLSN), if handleChangeFeedNotModified returns NO_RETRY on a noChanges page (single-partition case, multi-partition full-cycle complete, or the >4*(size+1) consecutive-304 defense in FeedRangeCompositeContinuationImpl), the SDK's built-in termination signal would otherwise be silently dropped. nextPageInternal now explicitly calls disableShouldFetchMore() when the SDK signals NO_RETRY on a noChanges page so the defense-in-depth termination guarantee survives for the new flag too. The noChanges(r) && emptyPagesAllowed guard is load-bearing because handleChangeFeedNotModified returns NO_RETRY for every data page too — without the guard, every data page would silently truncate iteration.

5. Spark ChangeFeedPartitionReader opts into the new flag via accessor.getImpl(options).setEmptyPagesAllowed(true).

6. CosmosChangeFeedRequestOptions.withCosmosPagedFluxOptions also propagates emptyPagesAllowed when the paged-flux pull mechanism supplies a continuation token (the freshly-built impl would otherwise silently lose the flag — added a comment flagging the broader drift hazard in that copy list).

7. DRY: extracted the two near-identical surface or swallow via repeatWhenEmpty blocks into a surfaceOrSwallowNoChangesPage(r) helper.

Behavior matrix

	Before	After
Spark query, sparse data	Single nextPage() blocks > 65 s, throws OCE	Each empty page surfaces; per-page timeout applies
Spark change feed, sparse data	Same OCE risk	Same fix applies
All other SDK callers	unchanged (emptyPagesAllowed defaults to false)	unchanged

[Copilot]

Pull request overview

This PR updates the Cosmos DB Java SDK and Cosmos Spark connector to optionally surface empty (304 / noChanges) change feed pages to callers, preventing Spark partition tasks from exceeding the connector’s 65s per-operation end-to-end timeout when draining sparse, cross-partition workloads.

Changes:

• Adds an internal emptyPagesAllowed flag for change feed requests and plumbs it through ChangeFeedQueryImpl → Paginator → ChangeFeedFetcher, so empty pages can be emitted instead of swallowed.
• Updates Cosmos Spark readers (query + change feed) to opt into “allow empty pages” so each physical page is bounded by the per-page timeout window.
• Adds new unit/regression tests covering the new behavior and the paged-flux continuation rebuild path; updates changelogs accordingly.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosChangeFeedRequestOptions.java	Preserves emptyPagesAllowed across paged-flux continuation rebuild; exposes internal accessor bridge methods.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/Paginator.java	Adds emptyPagesAllowed parameter and forwards it into ChangeFeedFetcher.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/ChangeFeedFetcher.java	Implements “surface empty pages” behavior and simplifies isFullyDrained to rely on continuation completion.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ImplementationBridgeHelpers.java	Extends change feed request options accessor interface with allow-empty-pages getters/setters.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/CosmosChangeFeedRequestOptionsImpl.java	Adds internal emptyPagesAllowed state and cloning support.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ChangeFeedQueryImpl.java	Passes allow-empty-pages option into the paginator pipeline.
sdk/cosmos/azure-cosmos/CHANGELOG.md	Documents the fix and clarifies Spark connector behavior.
sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/query/ChangeFeedFetcherEmptyPagesTest.java	Adds regression coverage for surfacing vs swallowing empty pages and new isFullyDrained semantics.
sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/CosmosChangeFeedRequestOptionsWithPagedFluxOptionsTest.java	Ensures allow-empty-pages survives withCosmosPagedFluxOptions continuation rebuild path.
sdk/cosmos/azure-cosmos-spark_3/src/main/scala/com/azure/cosmos/spark/ItemsPartitionReader.scala	Enables “allow empty pages” for Spark query reads.
sdk/cosmos/azure-cosmos-spark_3/src/main/scala/com/azure/cosmos/spark/ChangeFeedPartitionReader.scala	Enables “allow empty pages” for Spark change feed reads.
sdk/cosmos/azure-cosmos-spark_3/src/test/scala/com/azure/cosmos/spark/TransientIOErrorsRetryingIteratorSpec.scala	Adds Spark regression test for long empty-page runs without timing out.
sdk/cosmos/azure-cosmos-spark_3-5_2-13/CHANGELOG.md	Notes the Spark connector fix for sparse workloads.
sdk/cosmos/azure-cosmos-spark_3-5_2-12/CHANGELOG.md	Notes the Spark connector fix for sparse workloads.
sdk/cosmos/azure-cosmos-spark_3-4_2-12/CHANGELOG.md	Notes the Spark connector fix for sparse workloads.
sdk/cosmos/azure-cosmos-spark_3-3_2-12/CHANGELOG.md	Notes the Spark connector fix for sparse workloads.

[tvaron3]

/azp run java - cosmos - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[xinlian12]

@sdkReviewAgent

[kushagraThapar]

Automated multi-reviewer code review — Tier 3 (Deep)

Reviewers dispatched in parallel: PR Deep Reviewer (correctness) + Engineering Quality Reviewer (code quality / testing / perf / agentic) + cosmos-pr-reviewer (Cosmos design alignment) + Cross SDK Reviewer (peers: dotnet, python).
Risk score 8 (public_api +4, query+retry capped +6, source/test-ratio −2). Design-doc clone synced fresh against tvaron3/cosmosdb-design-docs@main. Existing inline comments from copilot-pull-request-reviewer were de-duped — all 3 were already addressed by the author in commit 08f690b.

---

Overall verdict

Approve with suggestions. Correctness story is solid — PR Deep Reviewer independently verified every claim in the PR body against the sandbox (10 claims, all ✅). Cross SDK Reviewer confirmed neither .NET nor Python has an equivalent flag (because neither ships CosmosEndToEndOperationLatencyPolicyConfig per-operation timeout) — Java is deliberately and correctly divergent. cosmos-pr-reviewer found no domain-correctness regressions, only design-doc gaps.

Findings cluster into four addressable themes: (1) stale PR description [major], (2) broader drift hazard in continuation-rebuild copy list [major], (3) test-timeout discipline to match .NET parity [major], (4) various minor API hygiene / observability / javadoc gaps.

---

Blockers

(none)

---

Major findings

M1. PR description references removed bridge accessor methods — stale and misleading

Files: PR body (sections 2 & 5)
Severity: major | Category: code_quality

PR body claims:

"Exposed via new package-private bridge accessor CosmosChangeFeedRequestOptionsAccessor.{get,set}AllowEmptyPages"

But commit 34d3da4fa51 ("Shrink bridge accessor surface") removed those wrapper methods. Spark now uses accessor.getImpl(options).setEmptyPagesAllowed(true) — the existing getImpl() escape hatch.

Verified:

$ grep -n "setEmptyPagesAllowed\|isEmptyPagesAllowed\|setAllowEmptyPages" \
    sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ImplementationBridgeHelpers.java
300:   void setAllowEmptyPages(CosmosQueryRequestOptions options, boolean emptyPagesAllowed);
302:   boolean getAllowEmptyPages(CosmosQueryRequestOptions options);
# only the pre-existing QUERY-side accessor; no change-feed-side wrapper

Why it matters: Future archaeologists triaging regressions will grep for accessor methods that don't exist.

Suggested fix: Update PR description items 2 & 5 to:

"Exposed via the existing accessor.getImpl(options).setEmptyPagesAllowed(...) escape hatch — no new bridge accessor wrappers are added (see commit 34d3da4fa51 for the rationale on keeping the accessor surface small for internal-only flags)."

---

M2. Drift hazard in withCosmosPagedFluxOptions extends well beyond emptyPagesAllowed

File: sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosChangeFeedRequestOptions.java:444-458
Severity: major | Category: design_alignment (with code-quality angle)

The PR adds a great drift-hazard comment ("this hand-maintained copy list is a known drift hazard") and propagates one field (emptyPagesAllowed) — but the same code path silently drops ~13 other fields when a user supplies a continuation token via byPage(savedContinuation):

endLSN, customSerializer, keywordIdentifiers, excludeRegions, readConsistencyStrategy, quotaInfoEnabled, customOptions, operationContextAndListenerTuple, thresholds, properties, isSplitHandlingDisabled, completeAfterAllCurrentChangesRetrieved, partitionKeyDefinition, collectionRid

Two of these are real Spark-adjacent footguns:

• endLSN — silently lost ⇒ a bounded change feed becomes unbounded after byPage(continuation) resume
• customSerializer — silently lost ⇒ custom type deserialization breaks across the continuation boundary

Scope caveat (this is 🟡 not 🔴): This is pre-existing; the PR's comment is genuinely useful and the author explicitly scoped broader cleanup as out-of-scope.

Suggested fix (pick one):

• Minimum: Open a tracking GitHub issue and reference it from the new in-code comment; list the 13 affected fields.
• Better (incremental win in this PR): Add endLSN and customSerializer to the copy list, plus two parallel tests in CosmosChangeFeedRequestOptionsWithPagedFluxOptionsTest mirroring the existing emptyPagesAllowed_isPropagated test.
• Best (follow-up PR): Invert the default — use the copy constructor new CosmosChangeFeedRequestOptionsImpl(this.actualRequestOptions) and surgically override only fields encoded in the continuation token. Changes the default from "drop unless listed" to "preserve unless overridden by the continuation".
• Also: consider a reflection-based test that iterates declared fields on the impl and asserts each survives the round-trip.

---

M3. Test-timeout discipline missing on regression-guard tests — .NET parity gap

File: sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/query/ChangeFeedFetcherEmptyPagesTest.java
Severity: major | Category: parity (test-rigor)

The new nextPage_emptyPagesAllowedTrue_* tests use StepVerifier.verifyComplete() with no explicit timeout. .NET's equivalent regression-guard test pins this with a wall-clock timeout so a future refactor reintroducing unbounded repeatWhenEmpty drain fails fast and loudly:

Verified:

$ grep -n "Timeout" Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/ChangeFeed/CrossPartitionChangeFeedAsyncEnumeratorTests.cs
173: [Timeout(5000)]   ... public async Task ShouldReturnNotModifiedAfterCyclingOnAllRanges(int partitions)

$ grep -n "timeOut\|Duration.of" sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/query/ChangeFeedFetcherEmptyPagesTest.java
# zero hits

Why it matters: Without a timeout, a regression would hang until TestNG's global timeout (often unset on unit suites) and look like CI flake rather than a loud failure. The PR explicitly motivates these tests as defense-in-depth against the very class of regression a wall-clock guard would catch.

Suggested fix: Add timeOut = 10_000 to the @Test annotation on the four nextPage_* tests, e.g.:

@Test(groups = { "unit" }, timeOut = 10_000)

OR replace .verifyComplete() with .expectComplete().verify(Duration.ofSeconds(5)). The isFullyDrained_* tests don't need this (synchronous pure method).

This is the single biggest test-rigor gap vs peer SDKs that Cross SDK Reviewer identified.

---

Minor findings

m1. Missing javadoc on new isEmptyPagesAllowed() / setEmptyPagesAllowed()

File: sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/CosmosChangeFeedRequestOptionsImpl.java:190-197
Category: code_quality

Sibling CosmosQueryRequestOptionsImpl.setEmptyPagesAllowed (line 312-320) documents "Defaults to false" and the param contract; the new methods have none. Three consumer sites (ChangeFeedFetcher plumbing, withCosmosPagedFluxOptions copy, Spark ChangeFeedPartitionReader) currently rely on tribal knowledge.

Suggested fix: 1-3 line javadoc: "When true, ChangeFeedFetcher surfaces 304/noChanges pages to the caller instead of swallowing them via repeatWhenEmpty; defaults to false. Caller iterators must handle empty FeedResponse pages."

---

m2. Bridge-accessor asymmetry: change-feed side reaches through getImpl() while query side has a wrapper

Files: ImplementationBridgeHelpers.java:300-302 + ImplementationBridgeHelpers.java:425-447 + ChangeFeedPartitionReader.scala:220-222 + ChangeFeedQueryImpl.java:124
Severity: minor | Category: code_quality

TL;DR: The query-side accessor exposes a first-class wrapper for this exact flag; the change-feed accessor doesn't, so the same logical opt-in is reached via two different code shapes. Grep-discoverability and refactor-fragility cost, not a correctness bug.

The two sides, side-by-side

Query side (pre-existing, untouched by this PR):

// ImplementationBridgeHelpers.java:300-302  — CosmosQueryRequestOptionsAccessor interface
void    setAllowEmptyPages(CosmosQueryRequestOptions options, boolean emptyPagesAllowed);
boolean getAllowEmptyPages(CosmosQueryRequestOptions options);

// ItemsPartitionReader.scala:54  — Spark call site, one-liner via accessor
.setAllowEmptyPages(queryOptions, true)

// ParallelDocumentQueryExecutionContext.java:319  — internal consumer uses the same wrapper
&& !qryOptAccessor().getAllowEmptyPages(this.cosmosQueryRequestOptions)

Change-feed side (added in this PR):

// ImplementationBridgeHelpers.java:425-447  — CosmosChangeFeedRequestOptionsAccessor
// (no setAllowEmptyPages / getAllowEmptyPages method on the interface)

// ChangeFeedPartitionReader.scala:220-222  — Spark call site reaches through getImpl()
.getImpl(options)
.setEmptyPagesAllowed(true)

// ChangeFeedQueryImpl.java:124  — internal consumer also reaches through getImpl()
changeFeedOptionsAccessor().getImpl(this.options).isEmptyPagesAllowed(),

Why it's worth flagging

1. Grep-discoverability is asymmetric. A future engineer running grep -rn 'setAllowEmptyPages\|getAllowEmptyPages' to answer "where do we opt callers into per-page empty-page surfacing?" gets only the query-side hits — Spark's change-feed call site and ChangeFeedQueryImpl are invisible at the symbol level. The cross-feature semantic equivalence disappears.
2. Refactor blast radius is asymmetric. Rename setEmptyPagesAllowed → something else later: query side breaks at the wrapper (single update point); change-feed side breaks at every getImpl().setEmptyPagesAllowed(...) call site (Spark + internal consumer).
3. The accessor interface is the friend-API contract. Once an option type has wrapper-style accessors for some flags and getImpl()-style reachthrough for others, the "what's the right way to access X from a friend module" question stops having a clean answer for that type.
4. Author has a defensible rationale. Commit 34d3da4fa51 ("Shrink bridge accessor surface") intentionally removed earlier wrapper additions to keep the accessor surface small. Real value worth honoring — but the cost is asymmetry vs. the sibling type that already exposes the same-named wrapper.

Suggested fix (recommend Option A; both are small)

• Option A (preferred) — mirror the query side. In ImplementationBridgeHelpers.java lines 425-447 add:

  void    setAllowEmptyPages(CosmosChangeFeedRequestOptions options, boolean emptyPagesAllowed);
  boolean getAllowEmptyPages(CosmosChangeFeedRequestOptions options);

  Wire them in CosmosChangeFeedRequestOptions.java (parallel to lines 769-778 of the query options file), and update ChangeFeedPartitionReader.scala:222 and ChangeFeedQueryImpl.java:124 to use the wrapper. Single grep-discoverable symbol across both option types.

• Option B — document the deliberate divergence. Keep the current shape but add a 2-line comment at the Spark call site explaining why this goes through getImpl() rather than getting its own wrapper, so future engineers don't "fix" the asymmetry by adding the wrapper anyway, and so they understand the surface-shrinking intent.

---

m3. PR body claims "see in-code comment" — no such comment exists

File: sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/CosmosChangeFeedRequestOptionsImpl.java:194-197
Category: code_quality

PR body: "deliberately no public setter — see in-code comment". Grep finds nothing.

Verified:

$ grep -rn 'deliberately no public\|public setter' \
   sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/CosmosChangeFeedRequestOptionsImpl.java \
   sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosChangeFeedRequestOptions.java \
   sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/ChangeFeedFetcher.java
# (no output)

The rationale for keeping the flag internal is the single most important note for future maintainers; should live in code, not the PR description (which decays).

Suggested fix: Add a 2-line comment on setEmptyPagesAllowed:

/* Intentionally not surfaced on the public CosmosChangeFeedRequestOptions API.
 * Caller must verify the iterator handles empty FeedResponse pages without
 * retry loops before opting in. */

---

m4. Test gap: surfaceOrSwallowNoChangesPage is invoked in BOTH branches, only streaming branch tested

File: sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/query/ChangeFeedFetcherEmptyPagesTest.java
Category: testing

The new helper is called from both branches of nextPageInternal:

• completeAfterAllCurrentChangesRetrieved || endLSN != null branch (line 140)
• streaming branch (lines 152, 165)

All 8 tests configure endLSN=null. Spark's ChangeFeedPartitionReader.scala:212-215 DOES set endLsn for bounded partitions, then unconditionally sets setEmptyPagesAllowed(true) — so the (endLSN != null, emptyPagesAllowed=true) combination is a real production scenario with zero unit coverage.

Verified:

$ grep -n "endLSN\|completeAfterAllCurrentChangesRetrieved" sdk/cosmos/azure-cosmos-tests/.../ChangeFeedFetcherEmptyPagesTest.java
339: /* completeAfterAllCurrentChangesRetrieved */ false,
341: /* endLSN */ null,

Suggested fix: Add 2 tests:

• nextPage_endLsnSet_emptyPagesAllowedTrue_surfacesNoChangesUntilHasFetchedAllChanges()
• nextPage_endLsnSet_emptyPagesAllowedFalse_swallowsNoChangesViaRepeatWhenEmpty() (legacy-mode regression guard)

---

m5. >4*(size+1) consecutive-304 defense only mock-tested at the fetcher layer

File: sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/query/ChangeFeedFetcherEmptyPagesTest.java:165-211
Category: testing

Every nextPage_* test mocks handleChangeFeedNotModified directly. The PR's defense-in-depth argument rests on the real FeedRangeCompositeContinuationImpl returning NO_RETRY after the >4*(size+1) threshold. Mock-based contract tests verify "IF NO_RETRY THEN terminate" but not "the real impl DOES return NO_RETRY after threshold".

Verified:

$ grep -n "when(continuation.handleChangeFeedNotModified" .../ChangeFeedFetcherEmptyPagesTest.java
131, 171, 219, 256: (all mocked, no integration with real impl)

Suggested fix: One integration-style test (still unit group, no real Cosmos client): wire a real FeedRangeCompositeContinuationImpl with 2-3 sub-ranges, feed >4*(size+1) consecutive 304 responses, assert shouldFetchMore() flips to false. Catches contract drift between ChangeFeedFetcher and FeedRangeCompositeContinuationImpl.

---

m6. Spark CHANGELOGs don't disclose per-page-overhead trade-off

Files: sdk/cosmos/azure-cosmos-spark_3-*_2-1*/CHANGELOG.md
Category: perf / agentic disclosure

All 4 Spark CHANGELOG entries announce the fix but don't document the per-page trade-off: workloads with many empty pages will now produce one Spark iterator callback per surfaced page (vs. previously one callback could drain many). Observability dashboards will see "pages per task" metrics spike; LLM cost-attribution tools will misattribute.

Verified:

$ grep -i 'emptyPagesAllowed\|allow.empty.page' sdk/cosmos/azure-cosmos-spark_3-*_*/CHANGELOG.md
# all 4 entries describe the fix but not the surfacing trade-off

Suggested fix: Append to each Spark CHANGELOG entry:

"Note: workloads with many empty pages will now observe one Spark iterator callback per surfaced page where previously a single callback could drain many — this is the intended trade-off for per-page timeout enforcement."

---

m7. New flag not reachable from public API — forward-looking design statement missing

File: sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/CosmosChangeFeedRequestOptionsImpl.java:194-197
Category: agentic

Opt-in flag is impl-class-only; a future Flink connector, MCP server, or 3rd-party agent that wants the same per-page timeout enforcement must depend on com.azure.cosmos.implementation bridge accessors.

Suggested fix: Either (a) graduate to a public @Beta method with doc-comment warning callers must handle empty FeedResponse pages, OR (b) explicitly document the forward-looking decision ("permanently internal — Spark connector only") in a code comment so future Cosmos team and 3rd-party consumers know which way the wind blows. (Overlaps with m3 — both fixes can ship in one code comment.)

---

m8. New surfaceOrSwallow decision invisible to logs/diagnostics/OpenTelemetry

File: sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/ChangeFeedFetcher.java:183-189
Category: agentic

The new surfaceOrSwallowNoChangesPage decision and the new NO_RETRY-on-noChanges branch are invisible to logs/diagnostics. An LLM diagnostician investigating "how many physical pages did this query drain" has no signal beyond the page count itself.

Suggested fix: Add structured trace:

if (logger.isTraceEnabled()) {
    logger.trace("change_feed.empty_page surfacing={} no_retry={} continuation={}",
                 this.emptyPagesAllowed,
                 retryResult == ShouldRetryResult.noRetry(),
                 getContinuationForLogging());
}

Structured key=value form per observability convention.

---

Nits

n1. nextPageInternal complexity at advisory boundary

File: ChangeFeedFetcher.java:121-176
Category: code_quality

56 lines, 4-deep nesting; the load-bearing noChanges(r) && this.emptyPagesAllowed guard is the 4th nested check. Dense comments mitigate but still right at the advisory boundary.

Suggested (optional): Extract the .flatMap((r) -> { ... }) body into a private applyNoChangesDecision(FeedResponse<T> r) method.

---

n2. Pre-existing no-op in CosmosEncryptionAsyncContainer:1158

Category: code_quality | Severity: nit (pre-existing, out-of-scope for this PR)

getEffectiveCosmosChangeFeedRequestOptions(pagedFluxOptions, finalOptions); discards the return value (the method has no side effects on its receiver). Encryption customers don't get even the partial copy-list propagation. Out of scope — file a follow-up.

---

n3. Test uses reflection where direct package access would work

File: ChangeFeedFetcherEmptyPagesTest.java:639-646
Category: testing

Test class and Fetcher.isFullyDrained live in the same package (com.azure.cosmos.implementation.query); reflection isn't needed. Reflection-based tests don't fail at compile time on rename refactors.

Suggested fix: Replace reflection with a tiny subclass:

class TestableChangeFeedFetcher extends ChangeFeedFetcher {
    boolean callIsFullyDrained(FeedResponse<Document> r) { return isFullyDrained(true, r); }
}

---

n4. No emulator / E2E test reproduces the actual OperationCancelledException scenario

Category: testing | Tag: unverified-pattern-suggestion

Coverage is unit-mock only. Granular mock coverage is appropriate primary defense, but an emulator test would lock in the fix at the integration boundary.

Verified:

$ grep -rln 'OperationCancelled\|end-to-end timeout\|endToEndTimeout\|emptyPagesAllowed' sdk/cosmos/azure-cosmos-tests/src/test
# only the two new files added in this PR

Suggested follow-up (not for this PR): @Test(groups = { "emulator" }) that creates a sparse multi-partition container with aggressive end-to-end-latency policy and asserts byPage().iterator() change-feed read completes without OCE.

---

Design alignment (citations to design docs)

cosmos-pr-reviewer raised 9 design-alignment findings, almost all of which are propose-design-doc-extension items.

D1. disableShouldFetchMore() bypasses Fetcher.update() top/maxItemCount machinery

File: ChangeFeedFetcher.java:160-169 | Citation: 12-change-feed.md#key-classes

Direct disableShouldFetchMore() call sidesteps the documented Fetcher base-class state machine. Benign because top is always -1 for change feed, but the bypass deserves an inline comment acknowledging the safety argument.

D2. surfaceOrSwallowNoChangesPage helper conflates snapshot and streaming mode semantics

File: ChangeFeedFetcher.java:183-189 | Citation: 12-change-feed.md#key-classes

DRYing the two original call sites risks silently coupling their swallow-vs-surface semantics. Suggested fix: add a comment stating the helper applies to both modes intentionally.

D3-D9. Propose-design-doc-extension items (for the cosmosdb-design-docs repo, NOT this SDK PR)

ID	Title	Target chapter	Proposed heading
D3	Incremental change-feed termination contract undocumented	12-change-feed.md	## Incremental Change Feed — Termination Contract
D4	>4*(size+1) consecutive-304 defense bail rationale lives only in code	12-change-feed.md	append to ### Scenario 4: Breadth-First 304 Cycling
D5	emptyPagesAllowed SDK contract surface (swallow vs surface) undocumented	12-change-feed.md + cross-ref 07-query-engine.md	## Empty-Page Handling — Swallow vs Surface
D6	07-query-engine.md silent on SDK-side ParallelDocumentQueryExecutionContext	07-query-engine.md	## SDK-Side Query Execution Contexts
D7	withCosmosPagedFluxOptions continuation-rebuild field drift is a design rule	12-change-feed.md	append ### Continuation-Token Round-Trip Field Drift
D8	17-status-codes-and-sdk-retries.md table missing change-feed 304 semantics cross-ref	17-status-codes-and-sdk-retries.md	row 304 entry — add change-feed cross-ref
D9	Merge-amplification of empty pages under emptyPagesAllowed=true not in Scenario 1	12-change-feed.md#scenario-1-duplicate-processing-after-merge	extend impact bullets

All 9 are non-blocking for this SDK PR. Treat as follow-up issues on the design-docs repo.

---

Cross-SDK informational divergences

These are not findings to act on, but the user (and any future cross-SDK consistency reviewer) should know:

Behavior	Java (post-PR)	.NET	Python	Note
Default 304 handling on cross-partition CF	repeatWhenEmpty (unbounded — pre-existing)	Aggregate per cycle, surface one NotModifiedPage	while True bounded by should_retry_on_not_modified_response()	Java still divergent in default mode — intentional back-compat preservation
Opt-in to surface per-page	emptyPagesAllowed=true (internal-only)	Not exposed (no equivalent flag)	Not exposed (no equivalent kwarg)	Verified by grep — zero hits in both peers
Per-operation end-to-end timeout config	CosmosEndToEndOperationLatencyPolicyConfig	Not present (only per-request AvailabilityStrategy)	Not present	This is why .NET/Python don't see the timeout bug
isFullyDrained-style termination signal	Yes (Java-specific)	No (uses cycle-bound IsNextRangeEqualToOriginal)	No (uses _initial_no_result_range cycle tracking)	The Java regression class has no peer analog
Empty cycle UX	Continues polling (Java)	Surfaces NotModifiedPage per cycle	Terminates iterator entirely (raise StopIteration)	Materially different — multi-SDK customers will observe different paging shapes

Cross SDK Reviewer's verdict: Approve. There is no peer SDK to copy the design from; Java's choices are internally consistent with its own pre-existing query-side setAllowEmptyPages and deliberately divergent from peers in a way peers cannot easily match without first shipping a per-operation timeout config.

---

Verified-negative findings (rigor receipts)

Preserved verbatim — these prove what was checked and explicitly ruled out:

• ❌ "Naming inconsistency emptyPagesAllowed vs allowEmptyPages at field/impl layer" — DROPPED. Field name + impl methods exactly match sibling CosmosQueryRequestOptionsImpl (verified by grep on line 25, 308, 317).
• ❌ "TransientIOErrorsRetryingIteratorSpec.scala only runs against azure-cosmos-spark_3 and not the 4 publishable variants" — DROPPED. All 4 variant pom.xmls reference <source>${basedir}/../azure-cosmos-spark_3/src/test/scala</source> (shared test source dir; auto-runs against all variants).
• ❌ "Coverage gap for >4*(size+1) defense in FeedRangeCompositeContinuationImpl" — Partially dropped. The NO_RETRY contract IS mock-tested at the fetcher layer (nextPage_emptyPagesAllowedTrueWithNoRetryOnNoChanges_terminatesIteration, lines 442-489). Finding m5 above re-raises the integration-against-real-impl gap, which is a complementary concern.
• ❌ "isFullyDrained default-mode regression coverage missing" — DROPPED. Test isFullyDrained_noChangesResponseWithEmptyPagesAllowedFalse_returnsTrue (line 379) exists and explicitly verifies short-circuit fires without consulting the continuation (Mockito.verify(continuation, Mockito.never()).isDone() — load-bearing pin).
• ❌ "Behavior change in (completeAfterAllCurrentChangesRetrieved || endLSN != null) branch for emptyPagesAllowed=false callers" — DROPPED. Compared git show HEAD~6:.../ChangeFeedFetcher.java to current: bytecode-equivalent for the legacy path (reEnableShouldFetchMoreForRetry(); return Mono.empty();). Behavior preservation verified.

---

What we explicitly did NOT flag

• PR Deep Reviewer confirmed (via 10/10 ✅ table) every claim in the PR body about: isDone() permanently false, handleChangeFeedNotModified NO_RETRY-for-data, only-one-caller verification on Paginator.getChangeFeedQueryResultAsObservable and new ChangeFeedFetcher(...), copy constructor includes new field, disableShouldFetchMore correctly stops Paginator, refactor behavior-preserving for all 4 (branch × flag) combinations.
• Existing copilot-pull-request-reviewer inline comments (3) — all addressed by author in commit 08f690b, dropped to avoid duplication.
• Multi-Spark-variant test coverage — verified shared source dir; no per-variant duplication needed.
• Bridge accessor pattern as language-idiomatic — verified parity with .NET InternalsVisibleTo (lists 20+ friend assemblies in AssemblyInfo.cs:7-44) and Python _module convention. Java's pattern is parity-aligned in spirit; not over-engineering.
• isFullyDrained regression as a Java-specific architecture issue — verified neither .NET nor Python has an analogous IsFullyDrained/IsDone() termination signal; the bug class has no peer analog.

---

Generated by the PR Review Router (Tier 3 — Deep) — orchestrating PR Deep Reviewer + Engineering Quality Reviewer + cosmos-pr-reviewer + Cross SDK Reviewer in parallel. Posted as a single review comment with explicit user approval after a human review pass.

[xinlian12]

✅ Review complete (26:15)

Posted 3 inline comment(s).

_{Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage}

[xinlian12]

does this mean we had a bug for changeFeed previously where we may not honor exclude regions etc?

[tvaron3]

Good question — yes, this rebuild path had a latent bug, but only on a niche entry point. Tracing the callers:

• The internal change-feed pipeline goes through ChangeFeedOperationState and the LRO Utils.getEffectiveCosmosChangeFeedRequestOptions paths — both pass a CosmosPagedFluxOptions whose requestContinuation is null on the first subscribe, so the if (pagedFluxOptions.getRequestContinuation() != null) branch is skipped and effectiveRequestOptions = this (the original). No field loss.
• The Spark connector builds fresh CosmosPagedFlux instances per partition and reads via iterator() — never hits the rebuild branch.
• The only public path that hits the rebuild branch is container.queryChangeFeed(opts).byPage(savedContinuationToken) (i.e. a caller chooses to resume from a previously-captured token).

Under the old code, that path silently dropped: endLSN, completeAfterAllCurrentChangesRetrieved, excludeRegions, readConsistencyStrategy, diagnosticsThresholds, customSerializer, keywordIdentifiers, maxPrefetchPageCount, throughputControlGroupName, and quotaInfoEnabled — anything not encoded in the continuation token. So yes, a caller resuming a change-feed read via byPage(savedToken) with excludeRegions configured would have silently lost the region pinning until the new page.

The inheritance fix here closes it for free (and the new CosmosChangeFeedRequestOptionsWithPagedFluxOptionsTest cases pin it).

[tvaron3]

/azp run java - cosmos - spark

[azure-pipelines]

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

[tvaron3]

/azp run java - cosmos - spark

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tvaron3]

/azp run java - cosmos - spark

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).