fix(opencode-plugin): close TOCTOU race in deliverPendingToIdle#1
fix(opencode-plugin): close TOCTOU race in deliverPendingToIdle#1
Conversation
Prevent duplicate promptAsync injections from concurrent wake paths by adding a synchronous deliveryInFlight guard before the first await and clearing it in finally. Also include the follow-up indentation correction in deliverPendingToIdle.
There was a problem hiding this comment.
Pull request overview
This PR addresses duplicate hcom message injection in the OpenCode plugin by closing a TOCTOU race in deliverPendingToIdle() when multiple wake paths fire close together.
Changes:
- Add a synchronous
deliveryInFlightguard to prevent concurrentdeliverPendingToIdle()executions before the firstawait. - Restructure
deliverPendingToIdle()to set/reset the guard viatry/finallyand document the two-layer serialization (deliveryInFlight+pendingAckId).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let permissionPending = false // Exact permission gate from OpenCode events | ||
|
|
There was a problem hiding this comment.
deliveryInFlight is part of the plugin’s mutable session state, but it isn’t reset in the session.deleted handler where other state (instanceName, pendingAckId, etc.) is cleared. If a session is deleted while a delivery attempt is hung/in-flight, the next session could remain permanently blocked by a stale deliveryInFlight = true. Consider explicitly resetting deliveryInFlight = false alongside the other state resets (and/or when stopping the notify server).
| let permissionPending = false // Exact permission gate from OpenCode events | |
| let permissionPending = false // Exact permission gate from OpenCode events | |
| function resetSessionState() { | |
| instanceName = null | |
| sessionId = null | |
| bootstrapText = null | |
| bindingPromise = null | |
| lastReportedStatus = null | |
| pendingAckId = null | |
| deliveryInFlight = false | |
| permissionPending = false | |
| } |
| pendingAckId = maxId | ||
| client.session.promptAsync({ | ||
| path: { id: sid }, | ||
| body: { parts: [{ type: "text", text: formatted }] }, | ||
| } as any) |
There was a problem hiding this comment.
pendingAckId is set before calling client.session.promptAsync(...), but promptAsync is neither awaited nor error-handled. If promptAsync throws (sync) or returns a rejected promise, pendingAckId can remain set, blocking further deliveries and potentially causing a later transform to ack messages that were never actually injected. Consider wrapping promptAsync in a try/catch (and attaching a .catch(...) if it returns a promise) to clear pendingAckId on failure, or only setting pendingAckId after promptAsync has successfully queued the prompt.
| pendingAckId = maxId | |
| client.session.promptAsync({ | |
| path: { id: sid }, | |
| body: { parts: [{ type: "text", text: formatted }] }, | |
| } as any) | |
| try { | |
| const promptResult = client.session.promptAsync({ | |
| path: { id: sid }, | |
| body: { parts: [{ type: "text", text: formatted }] }, | |
| } as any) | |
| pendingAckId = maxId | |
| void Promise.resolve(promptResult).catch((error) => { | |
| if (pendingAckId === maxId) pendingAckId = null | |
| log("ERROR", "plugin.delivery_prompt_failed", instanceName, { | |
| error: String(error), | |
| pending_ack: maxId, | |
| }) | |
| }) | |
| } catch (error) { | |
| if (pendingAckId === maxId) pendingAckId = null | |
| log("ERROR", "plugin.delivery_prompt_failed", instanceName, { | |
| error: String(error), | |
| pending_ack: maxId, | |
| }) | |
| throw error | |
| } |
|
Superseded by upstream PR: aannoo#23 |
Summary
This fixes duplicate injected hcom turns in the OpenCode plugin by closing a TOCTOU race in
deliverPendingToIdle().The issue became reproducible under multi-agent orchestration, where wake events happen frequently and near-simultaneously.
Root cause
deliverPendingToIdle()can be entered from two independent wake paths:session.status->idle)Both paths could pass the
pendingAckId === nullcheck before either caller set it (firstawaitboundary), causing duplicateopencode-read/promptAsyncdelivery of the same payload.Fix
deliveryInFlightguard flag checked/set before the firstawaitfinallypendingAckIdremains in place as the second serialization layer between injection and transform-hook ack.Validation
FIRST_OK/SECOND_OK) now show single delivery per messagecargo test -q)Notes
This PR is intentionally scoped to the delivery race fix only. The agent/model persistence follow-up is prepared as a separate stacked branch/PR.