Skip to content

feat(core): add opt-in buffering for raw invocation event logging - #398

Merged
ncipollina merged 6 commits into
mainfrom
feat/core-invocation-data-buffering
Sep 4, 2026
Merged

feat(core): add opt-in buffering for raw invocation event logging#398
ncipollina merged 6 commits into
mainfrom
feat/core-invocation-data-buffering

Conversation

@ncipollina

Copy link
Copy Markdown
Collaborator

Summary

  • Middleware couldn't safely log the raw Lambda event payload: IInvocationDataFeature.EventStream is read once, lazily, by event deserialization, and isn't guaranteed to be seekable — reading it in middleware would starve deserialization.
  • Adds an opt-in EnableBuffering() on IInvocationDataFeature (same idea as ASP.NET Core's HttpRequest.EnableBuffering()): buffers the event stream into a seekable MemoryStream only when a caller asks for it, so the cost is paid only by invocations that actually log the raw payload.
  • Adds ILambdaInvocationContext.EnableEventBuffering() as an ergonomic wrapper, matching the existing GetEvent<T>/GetResponse<T> extension pattern.

Changes

  • IInvocationDataFeature.EnableBuffering() — new method; no-ops if the stream is already seekable.
  • InvocationDataFeature.EnableBuffering() — copies the event stream into a MemoryStream, disposes the original, resets Position to 0.
  • ILambdaInvocationContext.EnableEventBuffering() extension in FeatureLambdaInvocationContextExtensions.
  • README: documents the pattern for logging raw request/response from middleware — buffer the event stream and reset Position after reading; the response stream can always be swapped since it's serialized to after the middleware pipeline completes.

Validation

  • dotnet build src/MinimalLambda/MinimalLambda.csproj — success, 0 warnings.
  • dotnet test tests/MinimalLambda.UnitTests (net10.0) — 545/545 passed, including new coverage for EnableBuffering (seekable no-op, non-seekable copy, content/position preservation, post-read reset) and the new context extension.
  • dotnet jb cleanupcode (repo formatter) run over the solution.

Notes for Reviewers

Existing EventStream behavior is unchanged for consumers who never call EnableBuffering() — factory still hands back the original stream as-is, so this is additive.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Qn9kagNWsCubTrnzN6NEqV

Middleware wanting to log the raw request payload previously had no
safe way to read IInvocationDataFeature.EventStream: it's read once,
lazily, by event deserialization, and isn't guaranteed to be seekable.
Reading it directly in middleware would starve deserialization.
Add EnableBuffering() (mirrors ASP.NET Core's HttpRequest.EnableBuffering)
which buffers the stream into a seekable MemoryStream only when needed,
plus an ILambdaInvocationContext.EnableEventBuffering() convenience
extension. Opt-in per invocation to avoid the copy when nobody is
logging.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn9kagNWsCubTrnzN6NEqV
@github-actionsgithub-actionsBot added the type: feat New feature label Sep 4, 2026

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:30094736fe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadsrc/MinimalLambda.Abstractions/Features/IInvocationDataFeature.cs Outdated
j-d-ha
j-d-ha previously approved these changes Sep 4, 2026

@j-d-haj-d-ha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggestion: could we consider making EnableEventBuffering() a context-level API, with the invocation-data feature keeping the implementation detail? It feels closer to the existing GetEvent<T>() / GetResponse<T>() helpers and would let middleware opt in without needing to know where the event stream is stored.

Current middleware usage:

lambda.UseMiddleware(async(context,next)=>{varinvocationData=context.Features.GetRequired<IInvocationDataFeature>();invocationData.EnableBuffering();usingvarreader=newStreamReader(invocationData.EventStream,leaveOpen:true);logger.LogInformation("Request: {Raw}",awaitreader.ReadToEndAsync());invocationData.EventStream.Position=0;awaitnext(context);});

Possible context-level usage:

lambda.UseMiddleware(async(context,next)=>{context.EnableEventBuffering();varinvocationData=context.Features.GetRequired<IInvocationDataFeature>();usingvarreader=newStreamReader(invocationData.EventStream,leaveOpen:true);logger.LogInformation("Request: {Raw}",awaitreader.ReadToEndAsync());invocationData.EventStream.Position=0;awaitnext(context);});

Not blocking—the feature-level API is coherent since it owns the stream.

ncipollinaand others added 2 commits September 4, 2026 08:26
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn9kagNWsCubTrnzN6NEqV
…ing' into feat/core-invocation-data-buffering
Adding EnableBuffering() directly to the public IInvocationDataFeature
interface source-breaks anyone who implements it themselves (custom
test hosts, hand-rolled fakes). Split it into a separate
IInvocationDataBufferingFeature, probed for via the feature collection
like ASP.NET Core's optional HTTP features, so IInvocationDataFeature
stays untouched and existing implementations keep compiling.
Addresses review feedback from chatgpt-codex-connector on PR #398.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn9kagNWsCubTrnzN6NEqV
@ncipollina

Copy link
Copy Markdown
CollaboratorAuthor

@codex review — please focus on the latest changes (commit 4e42345, the IInvocationDataBufferingFeature split addressing the earlier feedback) rather than a full repo review.

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:4e42345cb1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadsrc/MinimalLambda/Runtime/LambdaHandlerComposer.cs Outdated
j-d-ha
j-d-ha previously approved these changes Sep 4, 2026

@j-d-haj-d-ha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great work!

The prior fix registered IInvocationDataBufferingFeature under its own
key in the feature collection, alongside IInvocationDataFeature. Since
Features.Set<T> keys strictly by typeof(T), those became two
independent slots pointing at the same instance. Middleware replacing
IInvocationDataFeature (Features.Set<IInvocationDataFeature>(...))
would leave the buffering slot pointing at the stale instance, so
EnableEventBuffering() would silently buffer the wrong stream while
the actually-active one still gets consumed once by deserialization -
the exact starvation bug this feature exists to prevent.
Drop the separate registration. EnableEventBuffering() now resolves
whatever IInvocationDataFeature is currently active and probes it for
the capability, so there's no second slot to fall out of sync.
Addresses further review feedback from chatgpt-codex-connector on
PR #398.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn9kagNWsCubTrnzN6NEqV
@ncipollina

Copy link
Copy Markdown
CollaboratorAuthor

@codex review — please re-review the latest changes (commit 986bc80, resolving the buffering-desync feedback from the previous pass) rather than a full repo review.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit:986bc80eb3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@j-d-haj-d-ha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@ncipollina
ncipollina merged commit e7a8bc0 into mainSep 4, 2026
6 checks passed
@ncipollina
ncipollina deleted the feat/core-invocation-data-buffering branch September 4, 2026 12:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: featNew feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@ncipollina@j-d-ha