Repository navigation
fix(runtime): keep the newer event when the supersede log panics - #145
Conversation
A MarkEvent panic in the prelude aborts the request, but the delivery's dispatch span never ends, so tracing shows no terminal outcome for it.
A logger panic on the unrouted-message line comes after MarkEvent, so a fix keyed to the unfinished acceptance entry would miss it.
The prelude owns its span until it ends it on a resolved outcome or returns it in work. A panic does neither, so a deferred check on the zero results ends the span with the error outcome after the other releases run. The panic still reaches net/http. ADR 0015 invariant 4 gains the rule.
The supersede log runs on the displacing delivery's own goroutine before that delivery's run starts. When the app's slog handler panics there, the newer event never runs. Under debounce the displaced A runs instead of B. Under queue, C never runs after it takes the free lock, and a waiter displaced through registerPending is never superseded.
registerPending and takeQueueTurn now finish the handoff under queueMu without calling the app logger, and return the displaced waiter's event. The dispatcher writes the same supersede line once the newer delivery's run is secured: under DispatchDeferred right after its tail starts, and under DispatchSync after its handler returns. A panicking slog handler on that line still aborts the request, but the newer event runs and the displaced waiter ends superseded.
Signed-off-by: Thomas Kosiewski <tk@coder.com>
The prelude now opens the span, runs acceptAndRoute, and ends the span with the error outcome when acceptAndRoute did not return. That replaces the check on zero named results, which needed a comment to read.
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 73dd13dc99
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
Codex security review on #145: the newer delivery's tail copies work.supersedes, a pointer to the displaced Event. After the displaced tail frees its admission slot, its payload stays reachable until the newer tail ends, so it no longer counts against MaxDetached.
A fix that copies the displaced Event by value would free the Event but keep its Message and Raw payload reachable.
work.supersedes now holds a supersession with the displaced event's adapter, ID, and thread ID instead of the *Event. The newer tail no longer keeps the displaced payload reachable after the displaced tail frees its admission slot, so ADR 0015 invariant 1 holds again.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
The waiter comments said registerPending writes the superseded_by record. The displacing delivery's dispatcher writes it now. ADR 0015 invariant 4 gains the triage rule that a slog handler panic cannot drop an event the Concurrency Strategy keeps.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #143. The supersede log line ran on the newer delivery's own goroutine, after
MarkEventand before that delivery's run started. When the app'ssloghandler panicked on that line,net/httpaborted the request, the platform's retry was deduped, and the newer event never ran. The triage asked to finish the handoff before the log line, but that alone cannot help, because the goroutine that panics is the only one that would run the newer event. A delivery whose prelude panicked also left its dispatch span open, so the loss did not show in tracing.What changed
registerPendingandtakeQueueTurnfinish the handoff underqueueMuwithout calling the app logger. They return asupersessionwith the displaced event's adapter, ID, and thread ID, so the newer tail does not keep the displaced payload reachable. The dispatcher writes the samechat <strategy> supersededline once the newer run is secured. UnderDispatchDeferredthat is right after the tail starts. UnderDispatchSyncit is after the handler returns and the lock is released.preludenow opens the span, callsacceptAndRoute(the old prelude body), and ends the span with theerroroutcome whenacceptAndRoutepanicked. ADR 0015 invariant 4 names this, and gains the triage rule that asloghandler panic cannot drop an event the strategy keeps.Scope
net/http. This PR adds norecover.main.Tradeoffs
DispatchDeferredthe waiter runs on a detached tail, so its log panic crashed the test binary in every deferred logger-panic test. Under sync it failed the existingTestQueueSupersedeLoggerPanicReleasesItsThreadLockand left the waiter's span open.DispatchSyncnow appears after the newer handler ends.Blast Radius
Only the queue and debounce supersede paths and the prelude's panic path change. Every supersession still writes one line with the same message and keys. The line can now come after the displaced waiter's or the newer tail's own log lines.
Verification
httptest.Server. Before the fixes, debounce runs the displaced event instead of the newest one, the queue's newer event never runs, the displaced queue waiter is never superseded, and the span never ends.leaveQueueScopeassert. Logging before the run is secured also loses the event. Arecoverreturns 200 instead of aborting the request. An unconditional span end ends spans twice. A span end tied to the unfinished acceptance entry misses a panic afterMarkEvent.-race.go vetandgo testpass in every workspace module, andgo test -racepasses for the root package.Closes #143
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh