Skip IAST session tracking check when the servlet context is null (quick fix)

[dougqh]

What Does This Do

Fixes a NullPointerException in the IAST session-tracking advice when a request has no servlet context.

• GetHttpSessionAdvice.onExit (in both the javax servlet-3.0 and jakarta servlet-5.0 instrumentations) used request.getServletContext() as a context-store key without a null check. It now returns early when the context is null.
• Documents that ContextStore and WeakMap keys must not be null (Javadoc) and marks the key parameters @Nonnull.
• Adds a GetHttpSessionAdviceTest (JUnit 5) in each of the two servlet modules: it registers a recording IAST ApplicationModule, calls the advice with a request whose servlet context is null, and asserts nothing throws and the module is not called.

Motivation

Instrumentation telemetry shows an NPE at WeakMaps$Adapter.get (reached from FieldBackedContextStore.get), reported as "Failed to handle exception in instrumentation for ..." with IastOptOutJakartaHttpServletRequestInstrumentation$GetHttpSessionAdvice (and, on 1.61, the Atmosphere NoOpsRequest variant). It is about 2.9k events a day, from a couple of services.

Request copies such as Wicket's ServletRequestCopy and JavaxUpgradeHttpRequest, and Atmosphere's NoOpsRequest, return a session from getSession() but return null from getServletContext(). That null was passed to InstrumentationContext.get(...).get(context); the weak map behind the store rejects null keys and throws. Even with a tolerant store the advice would then have failed on context.getEffectiveSessionTrackingModes(), so the guard belongs in the advice. (That the throwing frame is the WeakConcurrentMap null-key check is inferred: its frame is redacted in the sample.)

Additional Notes

• Guard location: deliberately in the advice, not inside WeakMap. A silent null tolerance in WeakMap would not fix this bug, would only cover one of the three paths in FieldBackedContextStore.get (injected field, per-store weak map, global object store), and would hide null keys, which are bugs in our code, from telemetry.
• @Nonnull scope: documentation only; nothing enforces it at runtime. Static analysis will not have caught this bug because getServletContext() comes from the unannotated servlet API. SpotBugs and Spotless pass on the modules touched (internal-api, agent-bootstrap, both servlet modules); I did not run SpotBugs on every module that calls ContextStore, so a new finding at a distant call site would only show up in CI.
• Test approach: InstrumentationContext.get throws outside the agent, so on the unfixed advice the direct call fails, and it returns early with the fix. Confirmed for the javax module by reverting the guard; the jakarta advice is the same code. There is no agent-level test of the advice path.
• The jakarta module also relocates classes from the javax servlet-3.0 module; I fixed both sources and did not check whether the relocated copy duplicates the jakarta advice.

Contributor Checklist

• Format the title according to the contribution guidelines
• Assign the type: and (comp: or inst:) labels in addition to any other useful labels
• Avoid using close, fix, or any linking keywords when referencing an issue
  Use solves instead, and assign the PR milestone to the issue
• Update the CODEOWNERS file on source file addition, migration, or deletion
• Update public documentation with any new configuration flags or behaviors
• Once approved, use merge queue to merge the PR

Jira ticket: N/A

🤖 Generated with Claude Code

[datadog-prod-us1-4]

More details

The null guards in both servlet variants precede the context-store lookup and session-mode access while preserving the existing path for non-null contexts.

Was this helpful? React 👍 or 👎

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit f150065 · @DataDog review to ask questions}

[dd-octo-sts]

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.02 s	14.02 s	[-0.6%; +0.6%] (no difference)
startup:insecure-bank:tracing:Agent	12.93 s	13.01 s	[-1.3%; +0.0%] (no difference)
startup:petclinic:appsec:Agent	17.06 s	16.93 s	[-0.3%; +1.8%] (no difference)
startup:petclinic:iast:Agent	17.00 s	17.13 s	[-1.6%; -0.0%] (maybe better)
startup:petclinic:profiling:Agent	16.81 s	16.93 s	[-1.9%; +0.5%] (no difference)
startup:petclinic:sca:Agent	17.12 s	16.92 s	[+0.3%; +2.1%] (maybe worse)
startup:petclinic:tracing:Agent	16.28 s	16.26 s	[-0.8%; +1.1%] (no difference)

Commit: f1500652 · CI Pipeline · Benchmarking Platform UI

---

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

[ygree]

I think there's a slightly better way than just testing the advice directly. I've opened a stacked PR that only changes the tests #12696

[ygree]

The fix looks good. The test can be improved, as suggested in #12696