AsyncResultsMerger retry reuses stale opCtx-derived timeout/deadline when re-dispatching after reattach to a different OperationContext

XMLWordPrintableJSON

    • Type: Bug
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • None
    • Query Execution
    • ALL
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      TLDR

      there is a low chance if a response from a getMore comes back while detached and we try to refresh the opCtx, other fields might not be refreshed. like if an aggregate and getMore commands have different comments. or as the maxTimeMS is ticking down between getMore the acounting might not be right

      Issue

      When AsyncResultsMerger::_handleBatchResponse() schedules a delayed retry of a getMore, it
      captures a copy of the original RemoteCommandRequest (request = cbData.request). On
      re-dispatch it only reassigns the raw OperationContext pointer:

      async_results_merger.cpp:1436-1437

      auto refreshedRequest = request;
      refreshedRequest.opCtx = self->_opCtx;
      

      It does not recompute the opCtx-derived fields of the request. RemoteCommandRequest bakes
      timeout, deadline and timeoutCode in from the opCtx at construction time (see
      RemoteCommandRequest::_updateTimeoutFromOpCtxDeadline, which copies
      opCtx->getRemainingMaxTimeMillis(), opCtx->getDeadline() and opCtx->getTimeoutError()).
      The same applies to any opCtx-driven cmdObj decoration (comment, API parameters, tracing) that was
      serialized when the request was originally built.

      When this is reachable (realistic conditions)

      Each mongos getMore runs under a new OperationContext: between getMores the router cursor is
      checked in and the ARM detached (cluster_cursor_manager.cpp checkInCursor ->
      detachFromOperationContext), and on the next getMore it is checked out and reattached to the new
      opCtx (checkOutCursor -> reattachToOperationContext). So "reattach to a different opCtx" is
      the normal cursor flow.

      A mongos getMore's opCtx carries a deadline (making the shard request's deadline non-default)
      in one real flow: *non-tailable find/aggregate cursors whose originating command carried a
      maxTimeMS* – either client-specified, or injected fleet-wide by the defaultMaxTimeMS read
      default cluster parameter. The leftover budget is stashed on the router cursor
      (cluster_cursor_manager.cpp, registration) and re-applied to each getMore's opCtx at
      cluster_find.cpp (setUpOperationContextStateForGetMore, the
      getLeftoverMaxTimeMicros() branch), then baked into the ARM's shard request at
      async_results_merger.cpp _scheduleGetMoresForRemotes. No hand-crafted values are required –
      a cluster with defaultMaxTimeMS set produces per-getMore deadlines automatically.

      Not affected:

      • Tailable/awaitData (change stream) getMores – their maxTimeMS is applied as the awaitData
        timeout (waitForInsertsDeadline / a per-request maxTimeMS field), never as an opCtx
        deadline.
      • maxTimeMS on the getMore command itself – the generic mongos path
        (strategy.cpp) explicitly skips opGetMore when setting the deadline.
      • Multi-document transactions – no additional mongos opCtx deadline.

      Consequence

      If the retry is scheduled on the detached (_executor->sleepFor(...)) path – i.e. a getMore
      error response arrives while the ARM is detached – and the ARM is then reattached to the next
      getMore's opCtx before the backoff fires, the retry re-dispatches carrying the previous opCtx's
      deadline/timeoutCode. Both opCtxs have deadlines; the stale one is earlier (its budget
      started earlier and does not account for the wall-clock elapsed – including client think-time –
      between the two getMores). The re-dispatched shard getMore can therefore hit MaxTimeMSExpired
      prematurely, or immediately if the stale deadline has already elapsed. Severity is a spurious
      timeout on the retry, not data corruption or a crash.

      The attached/SubBaton path is not affected: detach routes the retry callback through the
      !self->_opCtx early-return guard, and a subsequent reattach rebuilds the request fresh via
      _scheduleGetMoresForRemotes(), so only the sleepFor path re-dispatches a stale captured
      request.

      Suggested fix

      Rebuild the RemoteCommandRequest against the currently-attached OperationContext before
      re-dispatch (the way _scheduleGetMoresForRemotes() constructs it), or otherwise recompute the
      opCtx-derived timeout/deadline/timeoutCode (and opCtx-driven cmdObj decoration), rather
      than copying the original request and patching only the opCtx pointer.

      Notes

      • Surfaced by the retry re-dispatch guard in SERVER-128736; the copy-and-patch predates and is
        unrelated to that fix's use-after-free correction. Filed separately per PR review.
      • A deterministic unit-test repro is on branch mjrb/arm-stale-deadline-repro (test
        AsyncResultsMergerTest.RetryAfterReattachToDifferentOpCtxUsesStaleTimeout in
        async_results_merger_test.cpp); it passes against master today, asserting the re-dispatched
        request carries opCtx A's earlier deadline rather than opCtx B's. Full source is in a comment
        below.
      • Affected file: src/mongo/s/query/exec/async_results_merger.cpp (retry re-dispatch in
        _handleBatchResponse).

            Assignee:
            Unassigned
            Reporter:
            Mickey Winters
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: