Repository navigation
fix(shutdown): handle runtimeDone without metrics; wait for the processor before the final flush - #1457
fix(shutdown): handle runtimeDone without metrics; wait for the processor before the final flush#1457DarcyRaynerDD wants to merge 3 commits into
Conversation
…rived After the shutdown drain, send the processor the SHUTDOWN reason. For a TIMEOUT or FAILURE shutdown in On-Demand mode, if the last invoked request never got a platform.runtimeDone, count its error (and timeout), and mark it so a runtimeDone can't count it again. Awaiting the processor's reply also guarantees the earlier fire-and-forget shutdown and OOM commands are handled before the final flush. Also handle platform.runtimeDone events without a metrics field instead of dropping them, which lost their error counts and stalled End-mode flushing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251e7180b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some(context) = self.context_buffer.get_mut(request_id) { | ||
| context.runtime_done_received = true; |
There was a problem hiding this comment.
When a tracer is active and update_reparenting runs after this metricless runtimeDone, runtime_done_received satisfies its send-readiness condition, so the trace agent returns this context to send_ctx_spans and emits the invocation span even though its runtime duration was never populated. This defeats the intended zero-length-span guard for that event ordering; tracking receipt separately from span readiness, or otherwise excluding this context from reparenting sends, would prevent it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. 7af598c moved the counting state off runtime_done_received so it no longer marked spans ready for reparenting. Since then, 44afb59 removed the SHUTDOWN-reason count entirely, because the integration suite showed it double counting. A metricless runtimeDone still leaves runtime_done_received false, and the test asserts that, so reparenting won't send that span.
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 1 Pipeline job failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 44afb59 | Docs | View more details | Give us feedback! |
runtime_done_received also marks the invocation span ready to send during reparenting, so setting it when counting from the SHUTDOWN reason or a metricless runtimeDone could send an unfinished span. Track the uncounted invocation on the processor instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The failure-accounting suite showed the count double counting: errors were 3-5 over on the default-strategy function in every attempt, while #1455 alone was exact. A runtimeDone that the closing listener refuses is redelivered to the restarted extension, which counts it, so counting it again from the SHUTDOWN reason was wrong. Keep the rest: platform.runtimeDone without metrics is still counted (without sending a zero-length span), and shutdown now waits on a processor barrier so earlier fire-and-forget commands are handled before the final flush. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DarcyRaynerDD
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
The timing-sensitive shutdown changes need the pending failure-accounting integration results reviewed before approval.
No open issues 🎉
What changed in this PR
Hardens shutdown handling on top of #1455 so the Lambda extension processes runtime completion events without metrics and waits for queued invocation-processor work before the final flush.
Changes:
- Counts failures from
runtimeDonestatus even when metrics are absent, while skipping span processing without a duration. - Adds a processor barrier and awaits it during shutdown before the final flush.
- Adds optional-metrics regression coverage and extends the no-op processor test to cover the barrier.
| File | Description |
|---|---|
bottlecap/src/lifecycle/invocation/processor.rs |
Handles optional runtime metrics and tests timeout counting without emitting a span. |
bottlecap/src/lifecycle/invocation/processor_service.rs |
Passes optional metrics through the service and adds the barrier command. |
bottlecap/src/bin/bottlecap/main.rs |
Forwards runtime completion events without metrics and awaits the shutdown barrier. |
Review effort: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44afb591b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let Some(metrics) = metrics else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
When an On-Demand tracer omits the request-ID header and its UniversalInstrumentationEnd is already buffered, a metricless runtimeDone returns here before pair_platform_runtime_done_event can consume that event. If the environment remains warm (for example, a successful runtimeDone whose optional metrics are absent), the next metric-bearing invocation consumes the stale response and associates its metadata/status with the wrong context, leaving the FIFO pairing permanently one invocation behind. Retire the buffered end event—or otherwise advance the pairing state—before skipping span completion.
Useful? React with 👍 / 👎.
Overview
Two small shutdown hardening changes on top of #1455.
platform.runtimeDonewithoutmetricsis no longer dropped. It was matched only asmetrics: Some(..), but AWS marksmetricsoptional, so such an event fell through to "Unforwarded Telemetry event".runtimeDonebefore calling/next.status. Span processing is skipped, because without a duration the invocation span would be sent with zero length.runtime_done_receivedstays false, so reparenting won't send it either.Barriercommand resolves once the processor has handled every earlier command. That includes the fire-and-forgetOnShutdownEvent(shutdown and unused-init metrics, the held-back cold-start invocation count) andOnOutOfMemoryError. Before, nothing guaranteed these were applied beforeflush_blocking_final.What changed since this PR was opened. It first also counted errors and timeouts from the SHUTDOWN reason (
TIMEOUTorFAILURE) when the in-flight invocation'sruntimeDonehadn't arrived. Thefailure-accountingsuite showed that overcounting on every attempt:endcontrol#1455 alone was exact on the same suite. A
runtimeDonethat the closing listener refuses is redelivered to the restarted extension, which counts it, so the SHUTDOWN-reason count was a second count. It has been removed.Stacked on #1455.
Testing
runtimeDonewithout metrics counts one error and one timeout, sends no span, and isn't marked ready for reparenting.Barriercommand.main.rsmatch-arm change.cargo test --workspace,cargo clippyandcargo fmtpass.failure-accountingsuite runs on this branch and should be exact on every attempt, as on fix(shutdown): send Tombstone after telemetry is drained #1455.🤖 Generated with Claude Code