Fix missing external memory accounting for OutgoingFactory fetchers

[dknecht]

Motivation

• A recent change centralized the external memory adjustment for subrequests into IoContext::getSubrequestNoChecks(), but OutgoingFactory-backed fetchers can bypass that path and thus stopped receiving any external memory accounting when the autogate is enabled, risking unaccounted C++ memory growth and OOM/DoS.

Description

• Detect whether fetch() obtained its client via the subrequest/traced path by checking clientWithTracing.traceContext and record this in hasSubrequestAdjustment.
• Preserve legacy behavior when INCREASE_EXTERNAL_MEMORY_ADJUSTMENT_FOR_FETCH is disabled by keeping the 3 KiB per-fetch adjustment.
• When the autogate is enabled, apply the 8 KiB adjustment in fetchImplNoOutputLock() only for clients that bypass the subrequest/tracing path (i.e., OutgoingFactory fetchers) to avoid double-accounting and close the accounting gap.
• Modified file: src/workerd/api/http.c++ (local change committed).

Testing

• Attempted to run Bazel test discovery (bazel query) and other Bazel-backed tests, but the environment could not download the Bazel binary (HTTP forbidden), so no automated Bazel tests were executed.

---

Codex Task

[ask-bonk]

Let me now do a final check: is traceContext really a kj::Maybe<TraceContext> in the ClientWithTracing struct?

Looking at the header we already read:

struct ClientWithTracing {
    kj::Own<WorkerInterface> client;
    kj::Maybe<TraceContext> traceContext;
};

Yes, it's kj::Maybe<TraceContext>. The != kj::none check is idiomatic and correct.

Now let me analyze if there are any issues:

1. Logic correctness: The heuristic (traceContext presence = went through subrequest path) is correct per the getClientWithTracing implementation. ✓
2. No double-counting: When autogate is enabled, subrequest-path fetchers get 8 KiB from getSubrequestNoChecks, and the new else if branch is skipped (hasSubrequestAdjustment is true). ✓
3. OutgoingFactory gap fixed: When autogate is enabled, OutgoingFactory fetchers get 8 KiB from the new else if branch. ✓
4. Legacy behavior preserved: When autogate is disabled, all fetchers get 3 KiB. ✓
5. No backward compatibility concerns: This is behind an autogate. ✓
6. No security concerns: This is a memory accounting fix, not a security boundary change. ✓

However, I want to flag one concern: the proxy variable hasSubrequestAdjustment is semantically misleading. It's actually checking "did the fetcher go through the subrequest/traced path" which happens to be where the centralized adjustment lives. But the name suggests it's a property of the request, not a property of the code path. If someone later changes getSubrequestNoChecks to not always apply the adjustment (or applies it conditionally), this heuristic would silently break. A comment explaining the correlation would be worthwhile.

Actually, wait - is there a more direct concern? Let me re-check: in getSubrequestNoChecks, the adjustment is conditional on currentLock:

if (util::Autogate::isEnabled(util::AutogateKey::INCREASE_EXTERNAL_MEMORY_ADJUSTMENT_FOR_FETCH)) {
    KJ_IF_SOME(lock, currentLock) {
      jsg::Lock& js = lock;
      ret = ret.attach(js.getExternalMemoryAdjustment(8 * 1024));
    }
  }

If currentLock is none, the centralized path does NOT apply the adjustment. But hasSubrequestAdjustment would still be true (because traceContext is non-none for subrequest paths), so the fetch-level adjustment would also be skipped. This could lead to no adjustment at all for subrequest-path fetches when currentLock is none.

However, in fetchImplNoOutputLock, we're always executing under a JS lock (the function takes jsg::Lock& js), so currentLock should always be set. The KJ_IF_SOME(lock, currentLock) guard in getSubrequestNoChecks is for the case when subrequests are initiated from non-JS contexts (infrastructure paths). Since fetchImplNoOutputLock is always called from JS, currentLock will be set, and the centralized adjustment will be applied. So this is not an actual issue.

The code change is correct and well-reasoned. The only minor issue is that using traceContext presence as a proxy for "has subrequest adjustment" is an indirect coupling. But it's documented in the comment, and the correlation is structural (inherent to the getClientWithTracing design), so it's acceptable.

LGTM

github run