Keep http.status_code when the Lambda AppSec context is missing

[joeyzhao2018]

What Does This Do

This is a review of #12587, delivered as a PR into its branch so the one code change I'm proposing is reviewable as a diff rather than described in prose. Merge it, cherry-pick it, or close it — the discussion items below need no code from me.

Two commits:

1. Keep http.status_code when the Lambda AppSec context is missing — the one behaviour regression I found, plus regression coverage.
2. Restore comment explaining the null trigger type check — a comment that looks like collateral damage from an edit.

Motivation

1. processResponseData's guard move silently made http.status_code conditional on AppSec

In LambdaAppSecHandler.processResponseData, the RequestContext guard moved from after the parse/status block to before the whole try, and was tightened to also require APPSEC slot data:

RequestContext requestContext = span.getRequestContext();
if (requestContext == null || requestContext.getData(RequestContextSlot.APPSEC) == null) {
  log.debug("Span has no AppSec request context, skipping response processing");
  return;
}

DDSpanContext.getRequestContext() returns this (DDSpanContext.java:841-843) and is never null for a real span, so the requestContext == null half was already effectively dead in production. The live half is the new APPSEC check.

That case is reachable. In processRequestStart, CURRENT_TRIGGER_TYPE is set to the real HTTP trigger type at line 99, and processAppSecRequestData runs at line 113 inside a try whose catch (Exception) at line 118 returns null. So if AppSec request processing throws, the span is created with an HTTP trigger type recorded but no AppSec context. On the way out, the new guard returns before ever reaching:

span.setHttpStatusCode(statusCode);
span.setError(isError, ErrorPriorities.HTTP_SERVER_DECORATOR);

Net effect: an AppSec failure at request start now also costs the invocation span its status code and error flag — tracing data that previously survived. That's the same coupling this PR's own commit 03452084 "Preserve Lambda resource name on AppSec failure" exists to prevent, which is why it reads as unintentional. It also runs against the grain of 68be58c0 "Infer status 200 for implicit-success Lambda responses" on master.

The fix

You can't keep the early exit and keep the status code — statusCode only exists after parseResponse, inside the try. So the guard is split into a flag plus a later gate: the tracing tag is published as before, and only the WAF callbacks are gated. This is the previous ordering with the stricter APPSEC condition retained.

RequestContext requestContext = span.getRequestContext();
boolean hasAppSecContext =
    requestContext != null && requestContext.getData(RequestContextSlot.APPSEC) != null;

...then, immediately before the callback block:

if (!hasAppSecContext) {
  log.debug("Span has no AppSec request context, skipping response WAF callbacks");
  return;
}

The response is parsed even when no AppSec context exists. That is exactly the pre-PR cost, not a new one, on a path that only happens when AppSec request processing already threw.

Evidence the regression is real

processResponseDataStillPublishesStatusWhenSpanHasNoAppSecContext was run against this PR's code without the production fix:

LambdaAppSecHandlerTest > processResponseDataStillPublishesStatusWhenSpanHasNoAppSecContext() FAILED

Wanted but not invoked:
agentSpan.setHttpStatusCode(503);
However, there was exactly 1 interaction with this mock:
agentSpan.getRequestContext();
-> at datadog.trace.lambda.LambdaAppSecHandler.processResponseData(LambdaAppSecHandler.java:191)

With the fix, it passes. The second assertion (verify(tracer, never()).getCallbackProvider(APPSEC)) keeps the original intent honest — the WAF callbacks must still be skipped, and the tracer must not even be consulted.

processResponseDataDoesNothingWhenSpanHasNoRequestContext is renamed to ...PublishesStatusButNoWafEventsWhenSpanHasNoRequestContext and given real assertions. Its body was // no exception expected — it asserted nothing, and its name described behaviour the span never had on master either. That empty assertion is why the change slipped through.

2. Restored comment

// A null trigger type means processRequestStart never ran, so the invocation was not analysed
// at all, which is not the same as an unsupported trigger.

Dropped while the requestContext guard below it was reworked. Both branches are still present and still behave differently, so the explanation still applies.

Additional Notes

Everything below is discussion, not code.

The bug #12587 fixes is more serious than its description claims

The description calls the pre-PR gap "could omit the existing WAF telemetry." It's more than that. GatewayBridge.onRequestEnded ends with ctx.close() (GatewayBridge.java:1110), and AppSecRequestContext.close() calls closeWafContext() — releasing the native ddwaf context. That code carries a telemetry log for exactly this situation:

log.debug(SEND_TELEMETRY, "WAF object had not been closed (probably missed request-end event)");

Pre-PR, a throwing handler never reached notifyAppSecEnd, so requestEnded never fired and the native WAF context was never released. Lambda reuses the execution environment across invocations, so that is native memory accumulating per throwing invocation for the life of the container, not just a missing metric.

Same shape for CURRENT_TRIGGER_TYPE: processRequestEnd removes it (line 132), so pre-PR it leaked across invocations on the reused handler thread. A later invocation where processRequestStart bailed at lines 80-83 (AppSec deactivated) — before the set at line 85 — would then read a stale trigger type in processResponseData.

Both are stronger justifications than what's written and worth two lines in Motivation; they change how a reviewer weighs the risk.

component: aws-lambda is only present on the AppSec path

applyHttpTags is reached only when AppSec is active, the trigger is HTTP, and processRequestStart produced a TagContext. For the telemetry goal that is sufficient — GatewayBridge:1127-1128 derives framework from Tags.COMPONENT on the span tags — but as a span tag component now appears inconsistently: present for AppSec HTTP invocations, absent otherwise. Nothing else sets it on this span (LambdaHandlerDecorator only holds the span name). Worth considering setting it unconditionally in the instrumentation and letting AppSec just read it.

Related: the tests now pin (LambdaAppSecHandlerTest:2706-2707) that inner instrumentation can overwrite component. So a Spring handler inside Lambda attributes API Security telemetry to spring-web-controller, not aws-lambda. Probably the right answer — but it means framework:aws-lambda is not guaranteed, which the description implies it is. One sentence would cover it.

New user-visible tag on the error path

testStreamingHandlerWithError now asserts _dd.appsec.unsupported_event_type = 1, which falls out of firing notifyAppSecEnd on the throw path. It is consistent with the non-throwing case, so it is correct — but it is a new tag appearing on error spans for non-HTTP-trigger Lambdas, and it is not mentioned in the description.

Test hygiene nit

GatewayBridgeSpecification.groovy:236 — if the as TagContext cast fails, processRequestEnd never runs and the CURRENT_TRIGGER_TYPE ThreadLocal leaks into later tests on that thread. A cleanup: clearing it would make it robust.

What's good

Worth saying, because the risky parts are handled well. Nulling result on the throw path is a clean way to reuse one call site for both outcomes — processResponseData's instanceof ByteArrayOutputStream check rejects it immediately. nestedHandlerFinalizesAppSecRequestOnce covers the real hazard of an unconditional notifyAppSecEnd (the call-depth guard). responseCallbacksReceiveNoDataWhenHandlerThrows with the new write-then-throw handler proves a partial response is not analysed. notifyAppSecEndFinalizesRequestWhenResponseProcessingThrows exercises the CoreTracer try/finally through a real CoreTracer rather than a mock. And moving setResourceName into finally is genuinely reachable — processRequestEnd has no try/catch, and processResponseData's catch is Exception, not Throwable.

Verification

Run locally on this branch (JDK 21):

Suite	Result
dd-trace-core datadog.trace.lambda.*	321 tests, 0 failures (LambdaAppSecHandlerTest 156)
aws-java-lambda-handler-1.2:test	LambdaHandlerInstrumentationV0Test 17, 0 failures
aws-java-lambda-handler-1.2:forkedTest	LambdaHandlerInstrumentationV1ForkedTest 17, 0 failures
appsec:test --tests '*GatewayBridgeSpecification*'	80 tests, 0 failures
dd-trace-core:spotlessJavaCheck	clean

🤖 Generated with Claude Code

[datadog-datadog-prod-us1-2]

More details

The change keeps HTTP status and error tags when the Lambda span has no AppSec context. It still blocks WAF callbacks without that context.

Was this helpful? React 👍 or 👎

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit 42f0e06 · @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.80 s	14.67 s	[-0.0%; +1.8%] (no difference)
startup:insecure-bank:tracing:Agent	13.53 s	13.66 s	[-1.6%; -0.2%] (maybe better)
startup:petclinic:appsec:Agent	17.54 s	17.39 s	[+0.1%; +1.6%] (maybe worse)
startup:petclinic:iast:Agent	17.41 s	16.87 s	[-1.3%; +7.6%] (no difference)
startup:petclinic:profiling:Agent	17.42 s	17.26 s	[-0.1%; +1.9%] (no difference)
startup:petclinic:sca:Agent	17.57 s	17.50 s	[-0.6%; +1.3%] (no difference)
startup:petclinic:tracing:Agent	16.59 s	16.65 s	[-1.2%; +0.5%] (no difference)

Commit: 42f0e064 · 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.