iframe-proxy

git-bruh · 2026-10-20T17:32:42Z

EventDispatcher.sendTraces() returns as soon as the trace handler has been queued as a waitUntil task, before it has waited for its isolate lock or run. A caller therefore cannot tell how long the receiver keeps working on its traces.

Add an optional completion capability to sendTraces(), of a new empty TraceCompletion interface. The receiver attaches it to the handler's IncomingRequest, so it is released once that request drains: the handler and its background work finish, time out, or abort. TraceCustomEvent carries the capability, and forwards the same one when it sends the event over RPC. A receiver that predates the field ignores it, releasing it on arrival.

clanky-the-clanker · 2026-10-20T17:34:42Z

💬 Approved with comments — The completion plumbing is correct: the capability is attached to the IncomingRequest before drain() on both the sendTraces and tailStreamSession paths, and sendRpc forwards it. What remains is one WARNING about missing tests and a few optional nits, none of which affects production.

Findings: 1 warning · 1 info · 2 suggestions

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #7855.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Code Quality] No tests cover the new `completion` capability on `sendTraces()` / `tailStreamSession()`. Nothing checks that it is released only after the request drains, that it is forwarded over RPC, or that omitting it still works.
- **File:** src/workerd/api/trace.c++:743

## INFO (1)

### [Documentation] The header comments say the completion is held until the handler's work is done. For actor receivers it is released when the request drains, which can happen while work continues. Align these comments with the caveat in the `TraceCompletion` schema comment.
- **File:** src/workerd/api/trace.h:755

## SUGGESTION (2)

### [AGENTS.md] Consider a short note in `src/workerd/io/AGENTS.md` on the TraceCompletion lifetime: it is released when the request drains, and for actors that can be before the work finishes.

### [Code Quality] `setCompletion()` on `TraceCustomEvent` and `TailStreamCustomEvent` has no callers in this tree, and the constructors and `fromRpc` already take the completion. Remove the setters, or document the caller that needs them.
- **File:** src/workerd/api/trace.h:794

📋 Code Quality: ⚠️ 1 warning · 1 suggestion

The plumbing is sound, but the new capability has no tests, and the two new setCompletion() setters have no callers in this tree.

Recommendation: 📝 Minor Comments

[WARNING] No tests cover the new completion capability on sendTraces() / tailStreamSession(). Nothing checks that it is released only after the request drains, that it is forwarded over RPC, or that omitting it still works. (src/workerd/api/trace.c++:743)
[SUGGESTION] setCompletion() on TraceCustomEvent and TailStreamCustomEvent has no callers in this tree, and the constructors and fromRpc already take the completion. Remove the setters, or document the caller that needs them. (src/workerd/api/trace.h:794)

View full review

The change is well structured. TraceCustomEvent::fromRpc and TailStreamCustomEvent::fromRpc set the completion only when hasCompletion() is true. Both run() implementations attach the completion to the IncomingRequest before drain(), so it is released only after the background work finishes, times out or aborts. sendRpc forwards the same capability. If an event is never run, the completion is dropped along with the event, which releases it as expected.

No test coverage for the new capability

The pull request adds a schema field and a new lifetime rule (the capability is held until IncomingRequest::drain() finishes), but changes no test files.
Nothing checks the following:
- The completion is released only after the handler and its waitUntil() work finish.
- The completion is forwarded unchanged over RPC.
- Sending without a completion still works.
Suggested test: use a TraceCompletion server whose destructor fulfils a promise. Dispatch sendTraces and tailStreamSession with it to a tail handler that uses waitUntil(). Assert that the promise is still pending while the handler runs and is fulfilled once the request drains.

Unused setters

setCompletion() at src/workerd/api/trace.h:794 and src/workerd/io/trace-stream.h:65 has no callers in this repository.
fromRpc and the constructors already take the completion.
If a caller outside this repository needs the setters, say so in a comment. Otherwise, remove them.

📖 AGENTS.md: 💬 1 suggestion

Optional: record the trace-completion lifetime rule in the I/O guidance.

Recommendation: 💬 Optional Nits

[SUGGESTION] Consider a short note in src/workerd/io/AGENTS.md on the TraceCompletion lifetime: it is released when the request drains, and for actors that can be before the work finishes.

View full review

src/workerd/io/worker-interface.capnp:926-935 defines when the trace completion is released, including the actor-specific caveat.
src/workerd/io/AGENTS.md already points contributors to the tracing files, so a short note on this rule there could help.

📝 Documentation: 💬 1 info

The header comments overstate when the completion is released for actor receivers.

Recommendation: 💬 Optional Nits

[INFO] The header comments say the completion is held until the handler's work is done. For actor receivers it is released when the request drains, which can happen while work continues. Align these comments with the caveat in the TraceCompletion schema comment. (src/workerd/api/trace.h:755)

View full review

src/workerd/api/trace.h:755-757 says the completion is kept "until the receiving tail Worker has finished handling the traces".
src/workerd/io/trace-stream.h:15-16 says it is kept "until the session's work is done".
The TraceCompletion schema comment correctly notes an exception: for an actor receiver, a later request can take over the waitUntil() tasks, so the capability can be released while work continues.
Change the header comments to say that the completion tracks request drain, or point them to that caveat.

✅ All other checks passed

🔒 Security: ✅ LGTM

No security concerns. The new capability has no methods and only signals completion by being released.

⚡ Performance: ✅ LGTM

No performance concerns. The completion adds at most one capability per event and changes no scheduling.

_{Reviewed commit: 8c5586cc}

ask-bonk · 2026-10-20T17:37:13Z

Since last review: 0 resolved, 1 still open, 0 new.

Adds completion-capability plumbing for buffered and streaming tail RPC delivery.

_{Reviewed commit: 8c5586cc · github run}

ask-bonk · 2026-10-20T17:37:16Z

+  // drain() keeps the request until its background work, including the handler above, finishes,
+  // times out, or aborts; the completion goes with it.
+  KJ_IF_SOME(c, completion) {
+    incomingRequest = kj::mv(incomingRequest).attach(kj::mv(c));


[WARNING] For an actor used as a dynamic tail target, another request can arrive while this tail handler or its event.waitUntil() work is still running. delivered() then fulfills this request's drainFulfiller, so drain() completes and destroys the attached capability, but actor waitUntil work remains in the shared IoContext task set for the newer request. The sender therefore observes completion before the handler's work has finished, contrary to the new completion contract. Keep the capability with per-event work that survives actor-request handoff, or reject actor tail targets until that lifetime can be represented.

EventDispatcher.sendTraces() returns as soon as the trace handler has been queued as a waitUntil task, before it has waited for its isolate lock or run. tailStreamSession() returns once the session completes, while the handler's waitUntil() work may continue. A caller therefore cannot tell how long the receiver keeps working on its traces. Add an optional `completion` capability to both calls, of a new empty TraceCompletion interface. The receiver attaches it to the handler's IncomingRequest, so it is released once that request drains: the handler and its background work finish, time out, or abort. TraceCustomEvent and TailStreamCustomEvent carry the capability, and forward the same one when they send the event over RPC. A receiver that predates the field ignores it, releasing it on arrival. For an actor receiver, a later request takes over the actor's background work, and the earlier request stops draining. The capability can then be released while the handler's work continues.

git-bruh requested review from a team as code owners October 20, 2026 17:32

ask-bonk Bot reviewed Oct 20, 2026

View reviewed changes

git-bruh force-pushed the pkhanna/trace-completion branch from 09c712d to cba0d22 Compare October 22, 2026 07:17

git-bruh requested a review from a team as a code owner October 22, 2026 07:17

git-bruh force-pushed the pkhanna/trace-completion branch from cba0d22 to ae67da6 Compare October 22, 2026 13:25

git-bruh force-pushed the pkhanna/trace-completion branch from ae67da6 to 8c5586c Compare October 22, 2026 15:37

jasnell approved these changes Oct 22, 2026

View reviewed changes

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Let sendTraces() callers learn when the handler finishes - #7855

Let sendTraces() callers learn when the handler finishes#7855
git-bruh wants to merge 1 commit into
mainfrom
pkhanna/trace-completion

git-bruh commented Oct 20, 2026

Uh oh!

clanky-the-clanker Bot commented Oct 20, 2026 •

edited

Loading

🔒 Security: ✅ LGTM

⚡ Performance: ✅ LGTM

Uh oh!

ask-bonk Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

ask-bonk Bot Oct 20, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

git-bruh commented Oct 20, 2026

Uh oh!

clanky-the-clanker Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📋 Code Quality: ⚠️ 1 warning · 1 suggestion

📖 AGENTS.md: 💬 1 suggestion

📝 Documentation: 💬 1 info

🔒 Security: ✅ LGTM

⚡ Performance: ✅ LGTM

Uh oh!

ask-bonk Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ask-bonk Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clanky-the-clanker Bot commented Oct 20, 2026 •

edited

Loading

ask-bonk Bot commented Oct 20, 2026 •

edited

Loading