Sync lambda deadline preservable#1032
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| err = status.Error(codes.InvalidArgument, "error: list entitlements requires a resource") | ||
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start), err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): A nil Resource now returns codes.InvalidArgument, whereas previously it fell through to the map lookup on an empty resource type and returned codes.NotFound. This is more correct, but per the SDK's status-code-stability guideline it's a callable-visible status change — worth a note in the PR description so downstream callers that branch on NotFound for this case are aware. ListGrants has the same change at line 339.
General PR Review: Sync lambda deadline preservableBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review Summary Security Issues Correctness Issues Suggestions
Prompt for AI agents |
…ut propagation - pkg/sync: IsSyncPreservable now returns true for codes.DeadlineExceeded and wrapped context.DeadlineExceeded errors, so checkpoint c1z files survive AWS Lambda hard timeouts and the sync can resume. - pkg/lambda/grpc: fix dead grpc-timeout check in TimeoutForRequest (len(v) > 1 could never match a single header value; now len(v) > 0). - pkg/lambda/grpc: clientConn.Invoke propagates the ctx deadline via the grpc-timeout header (encodeTimeout ported from grpc-go), returning DeadlineExceeded without invoking when the deadline already passed. Co-authored-by: Cursor <cursoragent@cursor.com>
3e5db5b to
96e513d
Compare
kans
changed the title
The len(v) > 0 fix made the server timeout path live for the first time. Lock in the behavior: a request with a grpc-timeout header yields a handler context carrying the deadline, a handler blocking past it produces a DeadlineExceeded status response, and requests without the header get no deadline. Co-authored-by: Cursor <cursoragent@cursor.com>
| func TimeoutForRequest(req *Request) (time.Duration, bool, error) { | ||
| v := req.Headers().Get("grpc-timeout") | ||
| if len(v) > 1 { | ||
| if len(v) > 0 { |
There was a problem hiding this comment.
Why was this 1? We were only checking for multiple grpc-timeout headers?
2 participants
Treat DeadlineExceeded as preservable and fix grpc-timeout propagation
Execution timeouts (e.g. a serverless connector hitting its platform's hard execution limit) currently cause syncs to throw away all checkpointed progress, and the lambda gRPC transport's deadline-propagation path doesn't actually work. This PR fixes both, so timed-out syncs resume from their checkpoint instead of restarting from zero.
Changes
1.
pkg/sync:IsSyncPreservablenow treats timeouts as preservableThe lambda transport already maps function timeouts to
codes.DeadlineExceeded(a retryable, resumable condition), butIsSyncPreservableomitted it from the allowlist, so the c1z checkpoint was discarded on timeout. This adds:codes.DeadlineExceededto the gRPC status allowlist.errors.Is(err, context.DeadlineExceeded)check for wrapped bare context deadline errors, which carry no gRPC status (status.FromErrordoes not mapcontext.DeadlineExceeded; onlystatus.FromContextErrordoes).2.
pkg/lambda/grpc: fix deadgrpc-timeoutcheck in the serverTimeoutForRequestread the header withmetadata.MD.Get, which returns[]string, then gated onlen(v) > 1— requiring two header values. A singlegrpc-timeoutheader (the only realistic case) never applied, making the server-side timeout path dead code. Nowlen(v) > 0.3.
pkg/lambda/grpc: client propagates the caller's deadlineclientConn.Invokenow resolves the long-standing TODO: if the context has a deadline, it sets thegrpc-timeoutheader (on a copy of the caller's metadata) so the server can bound the handler context; if the deadline has already expired it returnscodes.DeadlineExceededwithout invoking the function. AddsencodeTimeout(ported from grpc-go'sinternal/grpcutil, mirroring the existingdecodeTimeout).Tests
IsSyncPreservableunit tests: nil,ErrSyncNotComplete,ErrTooManyWarnings,Unavailable,DeadlineExceededstatus, wrappedcontext.DeadlineExceeded,Internal(false), plain error (false).TimeoutForRequestregression tests: single header applies, no header, malformed value errors.encodeTimeout/decodeTimeoutround-trips across unit ranges (1ns through 2,000,000h).clientConn.Invoke: header set when deadline present, absent otherwise, expired deadline short-circuits without calling the transport; caller metadata is not mutated.go build ./...,go test ./pkg/sync/... ./pkg/lambda/..., andmake lintall pass.Notes for reviewers
DeadlineExceededstatus through the invoker and persisting the c1z on timeout) lands separately.