Skip to content

lambda: restore ListStaticEntitlements tolerance and fix log-based failure classification #1073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
johnallers wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

lambda: restore ListStaticEntitlements tolerance and fix log-based failure classification #1073

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
198231a
lambda: restore ListStaticEntitlements tolerance and fix log-based cl…
johnallers Aug 10, 2026
88294dc
lambda: classify an absent RPC from the payload's own error type
johnallers Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 108 additions & 12 deletions pkg/lambda/grpc/failure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,20 @@ const (
// FailureClassHandled means the function returned an error response through
// the runtime API rather than crashing.
FailureClassHandled = "handled"
// FailureClassUnsupportedRPC means the connector could not resolve the
// request message's type URL against its proto registry, which is how an
// SDK older than the RPC fails. The RPC is absent, not broken.
FailureClassUnsupportedRPC = "unsupported_rpc"
)

// oomErrorType is the Error Type the AWS platform stamps on the REPORT line
// when it kills a sandbox for exceeding its memory limit.
const oomErrorType = "Runtime.OutOfMemory"

// protoResolveErrorType is the errorType a connector runtime reports when it
// cannot resolve a message's type URL against its proto registry.
const protoResolveErrorType = "prefixError"

@github-actions github-actions Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.


// LambdaInvokeFailure is a structured description of a failed Lambda invoke.
//
// Every field is derived from data the invoke already returns: the base64 tail
Expand Down Expand Up @@ -105,6 +113,13 @@ func (e *LambdaInvokeFailure) Code() codes.Code {
return codes.DeadlineExceeded
case FailureClassOOM:
return codes.ResourceExhausted
case FailureClassUnsupportedRPC:
// The connector predates the RPC, so every invoke fails identically
// until it is rebuilt. Unimplemented lets a caller skip the step the
// same way it would for any connector that does not serve the method,
// and survives error-string sanitization, which a substring match on
// the log summary does not.
return codes.Unimplemented
default:
return codes.Unknown
}
Expand All @@ -129,6 +144,8 @@ func (e *LambdaInvokeFailure) Error() string {
}
case FailureClassTimeout:
_, _ = b.WriteString("function timed out")
case FailureClassUnsupportedRPC:
_, _ = b.WriteString("function does not support this RPC")
default:
_, _ = b.WriteString("function returned error")
}
Expand Down Expand Up @@ -373,6 +390,15 @@ func extractMeaningfulLogLines(raw string) string {
func classifyLambdaFailure(functionError string, statusCode int32, payload []byte, rawLog string) *LambdaInvokeFailure {
report, haveReport := parseLambdaReportLine(rawLog)
errPayload := parseLambdaErrorPayload(payload)

// Two different views of the same log, on purpose.
//
// LogSummary is the one that may reach a customer-visible field, so it
// keeps the truncation pre-filter that drops a partial leading line.
// Classification reads the unfiltered lines instead: dropping a line is a
// sanitization decision, and a sanitization decision must never be able to
// change which failure class an invoke lands in.
signalLogs := extractMeaningfulLogLines(rawLog)
filteredLogs := extractMeaningfulLogLines(dropTruncatedFirstLine(rawLog))

failure := &LambdaInvokeFailure{
Expand All @@ -392,7 +418,7 @@ func classifyLambdaFailure(functionError string, statusCode int32, payload []byt
if haveReport && report.ErrorType != "" {
failure.ErrorType = report.ErrorType
}
failure.FailureClass = lambdaFailureClass(functionError, payload, filteredLogs, report, failure.ErrorType)
failure.FailureClass = lambdaFailureClass(functionError, payload, signalLogs, report, failure.ErrorType, failure.ErrorMessage)

return failure
}
Expand All @@ -402,29 +428,45 @@ func classifyLambdaFailure(functionError string, statusCode int32, payload []byt
// Timeout is checked first: a sandbox killed on its execution timeout can also
// show peak memory at its ceiling, which would otherwise trip the OOM
// memory-comparison fallback.
func lambdaFailureClass(functionError string, payload []byte, filteredLogs string, report lambdaReport, errorType string) string {
func lambdaFailureClass(functionError string, payload []byte, signalLogs string, report lambdaReport, errorType string, errorMessage string) string {
// Existing signal, unchanged: the platform writes this into the error
// payload on a hard timeout kill.
if strings.Contains(string(payload), "Task timed out after") {
return FailureClassTimeout
}
// Existing signal, unchanged: the function's own context deadline was
// 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
}
Comment on lines 460 to +475

@github-actions github-actions Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.


// Primary OOM signal, from the REPORT line or the error payload.
if errorType == oomErrorType {
return FailureClassOOM
}
// Fallback for runtimes that report the kill without an Error Type: a
// failed invoke whose peak memory reached its ceiling was an OOM.
if strings.EqualFold(report.Status, "error") &&
//
// Gated on the error type not being the function's own. A process the
// platform killed reports either nothing or a Runtime.* type such as
// Runtime.ExitError ("signal: killed"), so the ceiling is the best
// explanation available. When the function surfaced its own error value it
// 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") &&

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.)

report.MemorySizeMB > 0 && report.MaxMemoryUsedMB >= report.MemorySizeMB {
return FailureClassOOM
}
Expand All @@ -435,6 +477,38 @@ func lambdaFailureClass(functionError string, payload []byte, filteredLogs strin
return FailureClassUnhandled
}

// platformErrorTypePrefix marks the error types the AWS runtime generates
// itself, such as Runtime.ExitError and Runtime.OutOfMemory. Anything without
// it came from the function's own error value.
const platformErrorTypePrefix = "Runtime."

// isFunctionErrorType reports whether the error type came from the function
// rather than the platform. An absent type is not a function error: a hard kill
// leaves nothing behind.
func isFunctionErrorType(errorType string) bool {
return errorType != "" && !strings.HasPrefix(errorType, platformErrorTypePrefix)
}

// isUnresolvedTypeURL reports whether the runtime failed to resolve a message's
// type URL against its proto registry.
//
// Both the error payload's message and the log lines are searched because which
// one carries the text depends on how the runtime surfaced the failure. The
// type URL itself is deliberately not matched: every RPC added after a given
// connector's SDK version fails this same way, so pinning the check to one
// method name would leave the next one unhandled.
func isUnresolvedTypeURL(errorType string, errorMessage string, signalLogs string) bool {
if errorType != protoResolveErrorType {
return false
}
for _, s := range []string{errorMessage, signalLogs} {
if strings.Contains(s, "unable to resolve") && strings.Contains(s, "type.googleapis.com/") {

@github-actions github-actions Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

return true
}
}
return false
}

// lambdaLogTailTruncationThresholdBytes is the point at which a tail log may
// have been cut mid-line. AWS returns the last 4 KB of the execution log with
// LogType: Tail and cuts the window at a byte offset, not a line boundary, so a
Expand Down Expand Up @@ -493,20 +567,42 @@ func looksLikeLogLineStart(line string) bool {
return true
}
}
// A text-format runtime line, which the platform prefixes with an
// RFC3339 timestamp such as "2006-01-02T15:04:05.000Z".
// A text-format line, identified by the timestamp its writer puts in front.
return looksLikeTimestampPrefix(line)
}

// looksLikeTimestampPrefix reports whether a line opens with an RFC3339-style
// date, i.e. "NNNN-NN-NNT".
// timestampLayouts are the line-leading timestamp shapes that mark a whole log
// record, written with 'N' standing for any digit.
//
// Both entries matter. The AWS platform writes RFC3339, but the connector
// runtimes writing into this log use Go's standard logger, whose default prefix
// is "2006/01/02 15:04:05" - slashes and a space, not dashes and a "T".
// Recognising only RFC3339 classifies every one of those whole lines as a
// truncated fragment.
var timestampLayouts = []string{
"NNNN-NN-NNT",
"NNNN/NN/NN NN:NN:NN",
}

// looksLikeTimestampPrefix reports whether a line opens with a recognised
// timestamp layout.
func looksLikeTimestampPrefix(line string) bool {
const stamp = "NNNN-NN-NNT"
if len(line) < len(stamp) {
for _, layout := range timestampLayouts {
if matchesDigitLayout(line, layout) {
return true
}
}
return false
}

// matchesDigitLayout reports whether line starts with layout, where 'N' matches
// any digit and every other byte must match exactly.
func matchesDigitLayout(line string, layout string) bool {
if len(line) < len(layout) {
return false
}
for i := range len(stamp) {
want := stamp[i]
for i := range len(layout) {
want := layout[i]
got := line[i]
switch want {
case 'N':
Expand Down
106 changes: 106 additions & 0 deletions pkg/lambda/grpc/failure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,47 @@ func TestClassifyLambdaFailure(t *testing.T) {
wantCode: codes.Unknown,
wantLogSummary: "lambda-run: unexpected failure",
},
{
// A connector whose SDK predates the RPC cannot resolve the request
// message's type URL. The method is absent, not broken, so it maps
// to Unimplemented and the caller can skip the step.
name: "unresolved request type from an old connector sdk",
functionError: "Unhandled",
statusCode: 200,
payload: `{"errorMessage":"proto: (line 1:88): unable to resolve ` +
`\"type.googleapis.com/c1.connector.v2.ExampleServiceListExamplesRequest\": ` +
`\"not found\"","errorType":"prefixError"}`,
rawLog: "2026/05/22 21:50:47 unable to resolve type URL\n",

wantClass: FailureClassUnsupportedRPC,
wantCode: codes.Unimplemented,
wantErrorType: "prefixError",
wantLogSummary: "2026/05/22 21:50:47 unable to resolve type URL",
wantErrorMessage: `proto: (line 1:88): unable to resolve ` +
`"type.googleapis.com/c1.connector.v2.ExampleServiceListExamplesRequest": "not found"`,
},
{
// The memory fallback infers an OOM from peak memory reaching the
// ceiling. A function that surfaced its own error value already
// explained the failure, and a Go runtime routinely sits at its
// ceiling without being killed, so the ceiling proves nothing here.
name: "function error type at memory ceiling is not an oom",
functionError: "Unhandled",
statusCode: 200,
payload: `{"errorType":"prefixError","errorMessage":"malformed request"}`,
rawLog: "REPORT RequestId: abc-123\tDuration: 5000.00 ms\tBilled Duration: 5000 ms\t" +
"Memory Size: 128 MB\tMax Memory Used: 128 MB\tStatus: error\n",

wantClass: FailureClassUnhandled,
wantCode: codes.Unknown,
wantRequestID: "abc-123",
wantErrorType: "prefixError",
wantMemorySize: 128,
wantMaxMemory: 128,
wantDurationMS: 5000,
wantUtilization: 100,
wantErrorMessage: "malformed request",
},
}

for _, c := range cases {
Expand Down Expand Up @@ -449,6 +490,66 @@ func TestDropTruncatedFirstLine(t *testing.T) {
raw := strings.Repeat("x", lambdaLogTailTruncationThresholdBytes+10)
require.Equal(t, raw, dropTruncatedFirstLine(raw))
})

// Connector runtimes log through Go's standard logger, whose default prefix
// is "2006/01/02 15:04:05". Recognising only RFC3339 treated every one of
// those whole lines as a truncated fragment and dropped it.
t.Run("keeps a Go stdlib timestamped leading line in a full window", func(t *testing.T) {
var b strings.Builder
b.WriteString("2026/05/22 21:50:47 lambda-run: failed to get connector\n")
for b.Len() < lambdaLogTailTruncationThresholdBytes {
b.WriteString(`{"level":"debug","msg":"listing resources","duration_ms":12}` + "\n")
}
require.Equal(t, b.String(), dropTruncatedFirstLine(b.String()))
})
}

// TestClassifyLambdaFailureTruncationDoesNotChangeClass pins that sanitizing the
// log summary cannot change which class an invoke lands in.
//
// The truncation pre-filter drops a leading line it cannot recognise as a whole
// record. Classifying from that filtered text made the in-function timeout
// signal vanish whenever it landed on the first line of a full tail window,
// downgrading a retryable DeadlineExceeded into a terminal Unknown.
func TestClassifyLambdaFailureTruncationDoesNotChangeClass(t *testing.T) {
const deadlineMarker = `\"error\":\"context deadline exceeded\"`

fullTail := func(firstLine string) string {
var b strings.Builder
b.WriteString(firstLine)
b.WriteString("\n")
for b.Len() < lambdaLogTailTruncationThresholdBytes {
b.WriteString("2026/05/22 21:50:48 still listing resources\n")
}
b.WriteString(reportHealthy)
return b.String()
}

// A whole Go-stdlib line that the pre-filter used to misjudge as a fragment.
t.Run("whole leading line carrying the signal", func(t *testing.T) {
raw := fullTail(`2026/05/22 21:50:47 {"level":"error",` + deadlineMarker + `}`)
require.GreaterOrEqual(t, len(raw), lambdaLogTailTruncationThresholdBytes)

failure := classifyLambdaFailure("Unhandled", 200, nil, raw)
require.Equal(t, FailureClassTimeout, failure.FailureClass)
require.Equal(t, codes.DeadlineExceeded, failure.Code(),
"a timeout must stay retryable for the sync framework")
})

// A genuine mid-record fragment that still carries the signal. The pre-filter
// is right to keep this out of the summary and wrong to let that decision
// reach classification, so this case is covered by reading the raw log.
t.Run("truncated leading line carrying the signal", func(t *testing.T) {
fragment := `msg":"page timed out",` + deadlineMarker + `}`
raw := fullTail(fragment)
require.GreaterOrEqual(t, len(raw), lambdaLogTailTruncationThresholdBytes)

failure := classifyLambdaFailure("Unhandled", 200, nil, raw)
require.Equal(t, FailureClassTimeout, failure.FailureClass)
require.Equal(t, codes.DeadlineExceeded, failure.Code())
require.NotContains(t, failure.LogSummary, fragment,
"the fragment must still be kept out of the sanitizable summary")
})
}

// TestClassifyLambdaFailureTruncatedTailDoesNotLeak is the regression test for
Expand Down Expand Up @@ -483,6 +584,11 @@ func TestLooksLikeTimestampPrefix(t *testing.T) {
expected bool
}{
{line: "2026-05-22T21:50:47.000Z\tabc\tINFO\thello", expected: true},
// Go's standard logger default prefix, which connector runtimes emit.
{line: "2026/05/22 21:50:47 lambda-run: failed to get connector", expected: true},
{line: "2026/05/22 21:50:47", expected: true},
{line: "2026/05/22 21:50", expected: false},
{line: "2026/05/22", expected: false},
{line: "2026-05-22 21:50:47", expected: false},
{line: "202X-05-22T21:50:47.000Z", expected: false},
{line: "2026-05-22", expected: false},
Expand Down
21 changes: 18 additions & 3 deletions pkg/sync/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2108,6 +2108,12 @@ func (s *syncer) SyncStaticEntitlements(ctx context.Context, action *Action) err
return s.nextPageOrFinishAction(ctx, action, "", actions...)
}

// legacyUnresolvedStaticEntitlementsMarker is the raw lambda error text a
// connector too old for ListStaticEntitlements produces. It is a fallback for
// transports that do not classify the failure as codes.Unimplemented; prefer
// the status code, which survives error-string sanitization.
const legacyUnresolvedStaticEntitlementsMarker = `unable to resolve \"type.googleapis.com/c1.connector.v2.EntitlementsServiceListStaticEntitlementsRequest\": \"not found\"","errorType":"prefixError"`

func (s *syncer) syncStaticEntitlementsForResourceType(ctx context.Context, action *Action) error {
ctx, span := tracer.Start(ctx, "syncer.syncStaticEntitlementsForResource")
var err error
Expand All @@ -2123,10 +2129,19 @@ func (s *syncer) syncStaticEntitlementsForResourceType(ctx context.Context, acti
s.recordSessionUsage(resp.GetAnnotations())
s.recordConnectorWaitReport(resp.GetAnnotations(), action.ResourceTypeID)
if err != nil {
// Ignore prefixError if we're calling a lambda with an old version of baton-sdk.
if strings.Contains(err.Error(), `unable to resolve \"type.googleapis.com/c1.connector.v2.EntitlementsServiceListStaticEntitlementsRequest\": \"not found\"","errorType":"prefixError"`) {
// A connector built against an SDK older than this RPC cannot resolve
// the request message's type URL, so the method is absent rather than
// broken. Skip the step and let the rest of the sync run.
//
// The status code is the signal that holds. The substring below reads
// the connector's raw log text, which a caller may sanitize out of the
// error before it reaches here; it is kept only so this still works
// against a transport that does not yet map the failure to
// Unimplemented.
if status.Code(err) == codes.Unimplemented ||
strings.Contains(err.Error(), legacyUnresolvedStaticEntitlementsMarker) {
Comment on lines +2141 to +2142

@github-actions github-actions Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

l := ctxzap.Extract(ctx)
l.Info("ignoring prefixError when calling ListStaticEntitlements", zap.Error(err))
l.Info("connector does not support ListStaticEntitlements, skipping", zap.Error(err))
s.state.FinishAction(ctx, action)
return nil
}
Expand Down
Loading
Loading