lambda: restore ListStaticEntitlements tolerance and fix log-based failure classification - #1073
lambda: restore ListStaticEntitlements tolerance and fix log-based failure classification#1073johnallers wants to merge 2 commits into
Conversation
…assification #1048 gave every transport error path both the "lambda_transport:" prefix and the "logSummary:" separator so downstream sanitizers could strip connector log text from customer-visible fields. That closed a real leak, but the tolerance for connectors too old to serve ListStaticEntitlements was reading the raw log text out of err.Error() -- exactly what the sanitizer now removes. The substring stopped matching, the error propagated, and every sync of such a connector failed at that step instead of skipping it. Classify the failure instead of matching on it. A runtime that cannot resolve a request message's type URL reports errorType "prefixError"; that is a capability gap, not a crash, so it becomes FailureClassUnsupportedRPC and maps to codes.Unimplemented. The syncer keys off the code, which survives sanitization. The old substring stays as a fallback for transports that do not yet classify, and the check is no longer pinned to one method name -- every RPC added after a given connector's SDK version fails the same way. Two more classification defects from the same change: - dropTruncatedFirstLine ran before the log-based signals, so a sanitization decision could change a failure class. The in-function timeout marker disappeared whenever it landed on the first line of a full tail window, downgrading a retryable DeadlineExceeded into a terminal Unknown. Classification now reads the unfiltered lines; the summary keeps the filter. - looksLikeLogLineStart recognised RFC3339 but not "2006/01/02 15:04:05", the default prefix of Go's standard logger and therefore of nearly every line connector runtimes write. Whole lines were being dropped as fragments. Also gate the OOM memory-comparison fallback on the error type not being the function's own. A platform kill leaves nothing behind or a Runtime.* type; when the function surfaced its own error value it already explained the failure, and a Go runtime routinely sits at its memory ceiling without being killed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if status.Code(err) == codes.Unimplemented || | ||
| strings.Contains(err.Error(), legacyUnresolvedStaticEntitlementsMarker) { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): keying on codes.Unimplemented is broader than the old marker and will now silently swallow Unimplemented from sources unrelated to the SDK-version gap — uhttp.GrpcCodeFromHTTPStatus maps HTTP 501 to codes.Unimplemented (pkg/uhttp/wrapper.go:254), and pkg/connectorbuilder returns Unimplemented for misconfiguration (e.g. missing account manager / provisioner). A connector whose upstream API answers 501 during ListStaticEntitlements would get its static entitlements silently dropped from the sync with only an Info log, instead of failing loudly. Consider also requiring the failure to look like a capability gap (e.g. errors.As to *lambdagrpc.LambdaInvokeFailure with FailureClass == FailureClassUnsupportedRPC, falling back to the code), or at least logging this at Warn.
| return false | ||
| } | ||
| for _, s := range []string{errorMessage, signalLogs} { | ||
| if strings.Contains(s, "unable to resolve") && strings.Contains(s, "type.googleapis.com/") { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the two strings.Contains calls are evaluated independently against the whole joined multi-line string, so "unable to resolve" on one log line and "type.googleapis.com/" on a completely unrelated line satisfy the check together. Given that a false positive here maps to codes.Unimplemented, and Unimplemented is silently tolerated in two places in the syncer (getResourceFromConnector returns nil, nil at pkg/sync/syncer.go:1409, and syncStaticEntitlementsForResourceType skips the step), a false positive turns a real failure into silently missing data. Consider scoping the match to errorMessage only, or requiring both substrings on the same line.
| // exhausted and the connector logged it. | ||
| if strings.Contains(filteredLogs, `\"error\":\"context deadline exceeded\"`) { | ||
| if strings.Contains(signalLogs, `\"error\":\"context deadline exceeded\"`) { | ||
| return FailureClassTimeout | ||
| } | ||
| // New signal: newer runtimes stamp the outcome on the REPORT line. | ||
| if strings.EqualFold(report.Status, "timeout") { | ||
| return FailureClassTimeout | ||
| } | ||
|
|
||
| // A connector whose SDK predates the RPC cannot resolve the request | ||
| // message's type URL. Checked before the OOM signals because it is an exact | ||
| // match on the runtime's own verdict, where the memory fallback below is | ||
| // inferred. | ||
| if isUnresolvedTypeURL(errorType, errorMessage, signalLogs) { | ||
| return FailureClassUnsupportedRPC | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the in-function deadline signal is read from the tail log, which this file already documents as possibly containing a previous invoke's output on a warm sandbox (see parseLambdaReportLine), and switching it to signalLogs widens that window by no longer dropping the truncated leading line. It runs before isUnresolvedTypeURL, which reads the current invoke's own error payload. So an old connector whose warm tail happens to carry a stale \"error\":\"context deadline exceeded\" classifies as timeout/DeadlineExceeded, the Unimplemented mapping never happens, and the ListStaticEntitlements tolerance this PR restores does not apply. Payload-derived signals are stronger than tail-log-derived ones; consider running isUnresolvedTypeURL ahead of the log-based deadline check.
|
|
||
| // protoResolveErrorType is the errorType a connector runtime reports when it | ||
| // cannot resolve a message's type URL against its proto registry. | ||
| const protoResolveErrorType = "prefixError" |
There was a problem hiding this comment.
🟡 Suggestion (low-medium confidence): "prefixError" is the unexported struct name of google.golang.org/protobuf/internal/errors.prefixError (vendor/google.golang.org/protobuf/internal/errors/errors.go:24), surfaced only because aws-lambda-go stamps the Go type name into errorType. A rename or refactor in a routine protobuf-go bump silently reverts this whole fix to the original bug, with no compile error and no test failure. Worth noting the vendored source in the comment so a dependency bump has a chance of flagging it, and consider whether the "unable to resolve" + "type.googleapis.com/" text match alone is sufficient without the exact-type gate.
| // failChildResourceMockConnector wraps a mockConnector and fails ListResources calls for child resources. | ||
| // staticEntitlementsErrorMockConnector fails ListStaticEntitlements with a | ||
| // caller-supplied error, standing in for a connector whose SDK predates the RPC. |
There was a problem hiding this comment.
🟡 Suggestion: the new type was inserted directly under the pre-existing failChildResourceMockConnector doc comment with no blank line, so line 1840 now reads as part of staticEntitlementsErrorMockConnector's doc block and failChildResourceMockConnector (line 1856) is left undocumented. Move line 1840 back above type failChildResourceMockConnector.
General PR Review: lambda: restore ListStaticEntitlements tolerance and fix log-based failure classificationBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit splits the resolved error type from the payload's own so a Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Review follow-up. classifyLambdaFailure resolves ErrorType with a REPORT line's platform verdict winning over the payload's, and then handed that resolved value to the entire classifier. But an absent RPC is a statement only the function can make, so a REPORT line carrying any Error Type -- Runtime.ExitError, say -- overwrote the payload's prefixError, isUnresolvedTypeURL stopped matching, and the invoke fell through to unhandled/Unknown. That silently restores the bug this branch exists to fix, with no test failure to show it. Pass the payload's error type alongside the resolved one and use it for the unresolved-type-URL check only. The OOM signals keep the resolved value, where the platform's verdict is the right one to trust, and the ErrorType field keeps reporting the platform's verdict because that is what the field means. The shape seen in practice carries no REPORT Error Type for this failure, so this closes a latent hole rather than a live one. The new case was validated as an instrument: restoring the old argument fails it, and only it. Also move a doc comment the previous commit orphaned -- failChildResourceMockConnector's godoc had ended up attached to the new staticEntitlementsErrorMockConnector.
| // has already explained the failure, and peak memory sitting at the ceiling | ||
| // is a coincidence - a Go runtime routinely runs at its ceiling without | ||
| // being killed. | ||
| if !isFunctionErrorType(errorType) && strings.EqualFold(report.Status, "error") && |
There was a problem hiding this comment.
🟡 Suggestion: this commit split the resolved and payload error types precisely because "a REPORT line carrying any Error Type would otherwise overwrite" the function's own verdict — but the OOM gate still reads the resolved errorType, so the same masking applies to it. The new test fixture on line 337 establishes that payload errorType: prefixError + REPORT Error Type: Runtime.ExitError is a real shape; with Max Memory Used >= Memory Size that shape makes isFunctionErrorType(errorType) false and the invoke is classified OOM (ResourceExhausted, "function ran out of memory") even though the function surfaced its own error value — exactly what the gate was added to prevent. Consider gating on !isFunctionErrorType(payloadErrorType), or at minimum add that permutation to the table so the choice is pinned. (Minor, same doc block: "payloadErrorType is the function's own, always" isn't quite true — the existing OOM fixtures carry the platform's Runtime.ExitError in the payload.)
1 participant
What broke
#1048 made every lambda transport error path emit both the
lambda_transport:prefix and thelogSummary:separator, so downstream sanitizers could strip connector log text out of customer-visible fields. That closed a real leak.It also broke the tolerance added in #560 for connectors too old to serve
ListStaticEntitlements. That check matched a substring of the connector's raw log text viaerr.Error()— precisely what a sanitizing caller now removes. Before #1048 the prefixError path returned bare log lines carrying neither marker, so it slipped past sanitization and the substring matched. After, it is sanitized to a generic message, the match fails, the error propagates, and the sync fails at that step instead of skipping it.This affects every connector built against baton-sdk ≤ v0.5.21 — anything older than the release that introduced the RPC. Those runtimes cannot resolve
EntitlementsServiceListStaticEntitlementsRequestand reporterrorType: prefixError.Impact in production
This is live, not hypothetical. Measured across the rollout of the release that vendored #1048 into the invoking service:
ignoring prefixErrorlog ran steadily at roughly 1.4k/day, then went to 0 in the same hour the rollout landed, and has stayed at 0 every day since.The two counts are complements of each other: the same population moved from "tolerated, logged at info" to "propagated as a sync failure" at the release boundary, which is what identifies the cause as this change rather than any shift in connector behaviour.
The fix
Classify the failure instead of matching on it. A runtime that cannot resolve a request message's type URL is reporting a capability gap, not a crash. It now becomes
FailureClassUnsupportedRPC→codes.Unimplemented, and the syncer keys off the code, which survives sanitization. The old substring stays as a fallback for transports that do not yet classify. The check is no longer pinned to one method name — every RPC added after a given connector's SDK version fails identically, so pinning it would leave the next one unhandled.Two more classification defects from the same change
Sanitization could change a failure class.
dropTruncatedFirstLineran before the log-based signals. The in-function timeout marker ("error":"context deadline exceeded") vanished whenever it landed on the first line of a full tail window, downgrading a retryableDeadlineExceededinto a terminalUnknownand costing the sync framework its retry-and-checkpoint. Classification now reads the unfiltered lines;LogSummarykeeps the filter, since dropping a line is a sanitization decision and must not be able to move a failure class.looksLikeLogLineStartonly recognised RFC3339. Connector runtimes log through Go's standard logger, whose default prefix is2006/01/02 15:04:05— slashes and a space, not dashes and aT. Every one of those whole lines was being treated as a truncated fragment and dropped on any tail ≥ 4 KB.OOM memory fallback could false-positive. Now gated on the error type not being the function's own: a platform kill leaves nothing behind or a
Runtime.*type, whereas a function that surfaced its own error value already explained the failure — and a Go runtime routinely sits at its ceiling without being killed.Companion change required
This is inert on its own for callers that sanitize.
mapInvokeErrorin the platform repo preserves onlyDeadlineExceeded; every other classification collapses to a generic infra error, soUnimplemented(and theResourceExhaustedthat #1048 introduced for OOM) never reaches the sync framework. That repo also never recovers*LambdaInvokeFailureviaerrors.As, so the structured fields are only ever visible as substrings.The companion change is now open and green. It routes on the typed failure via
errors.Asand returns the connector's own error text for the connector-authored case, which incidentally restores the substring fallback here as well — so the two changes are belt-and-braces on the tolerance and can land in either order. Note the ordering consequence: while that fallback is load-bearing, the consumer cannot truncate or reshapeLogSummarywithout re-breaking the tolerance. Once this PR merges the syncer keys offcodes.Unimplementedinstead, and that constraint goes away.Testing
make lintclean,./pkg/sync/... ./pkg/lambda/...green. Each new test was validated as an instrument — reverted its fix and confirmed the test fails:TestSyncToleratesUnimplementedStaticEntitlementsTestClassifyLambdaFailureTruncationDoesNotChangeClassTestLooksLikeTimestampPrefix/TestDropTruncatedFirstLineTestClassifyLambdaFailure(2 new cases)Unimplemented; function error type at the memory ceiling is not an OOMExisting
TestClassifyLambdaFailure/oom_from_memory_at_ceiling_without_error_typestill passes —Runtime.ExitError/signal: killedat the ceiling remains a genuine OOM.Independent re-verification of the two classification defects
Both were re-confirmed after the fact by running one identical throwaway test against
mainand against this branch. Defect 1 uses byte-identical log content in both rows — only the tail length differs, so tail size alone is what moves the class:maintimeout/DeadlineExceededtimeout/DeadlineExceededunhandled/Unknowntimeout/DeadlineExceededmain2006/01/02 15:04:05(Go stdlib default)2006-01-02T15:04:05Z(RFC3339)Defect 2 is the mechanism behind Defect 1: the marker line carrying the in-function timeout signal is written by Go's standard logger, so it was the line being discarded.