fix(reply): prevent hung pre-delivery hooks from blocking lanes#104256
Conversation
37f4152 to
f7c8567
Compare
|
Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 6:28 PM ET / 22:28 UTC. Summary PR surface: Source +508, Tests +915, Docs +13, Generated 0, Other -1. Total +1435 across 24 files. Reproducibility: yes. at source level: an unresolved pre-delivery callback retains the serialized send chain and prevents Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge this single owner-aware timeout and intent-safe reconciliation path after the final required exact-head check succeeds, retaining the documented per-stage override for legitimately slow callbacks. Do we have a high-confidence way to reproduce the issue? Yes at source level: an unresolved pre-delivery callback retains the serialized send chain and prevents Is this the best way to solve the issue? Yes. Per-stage owner-overridable deadlines preserve legitimate slow work better than an aggregate timeout, while delivery-outcome and intent-identity reconciliation avoids cancellation and blind pending-state clearing. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8a4463eef1b9. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +508, Tests +915, Docs +13, Generated 0, Other -1. Total +1435 across 24 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (9 earlier review cycles; latest 8 shown)
|
625b270 to
59e7daf
Compare
59e7daf to
d0cc47d
Compare
d0cc47d to
0dcfd5f
Compare
0dcfd5f to
888124a
Compare
|
Maintainer land-ready pass complete at What changed during review:
Proof:
Proof gap: no credentialed live WhatsApp smoke. The defect and fix sit in the channel-agnostic serialized reply-lane owner, and the focused lifecycle tests exercise the hung-callback, timeout, queue-release, retry, stale-intent, and transport-start branches directly. Ready to merge. Thanks @NianJiuZst for the original fix and tests. |
888124a to
fa43be8
Compare
fa43be8 to
ba2ea9d
Compare
|
Merged via squash.
|
…claw#104256) * fix(reply): bound pre-delivery hook settlement * test(plugins): preserve hook timeout fixtures * fix(reply): preserve declared pre-delivery budgets * chore(reply): finalize pre-delivery recovery * fix(reply): preserve per-final recovery semantics * fix(reply): guard pending-final settlement identity * test(reply): align pending-final store fixture * fix(reply): bind settlement to originating intent * test(reply): satisfy timeout fixture lint * chore(plugin-sdk): refresh rebased baseline * fix(reply): preserve normalized retry ownership * fix(reply): narrow pending retry metadata * fix: align reply dispatch state access * chore(plugin-sdk): refresh rebased surface baseline * test(reply): align rebased accessor assertion * chore(plugin-sdk): regenerate drifted API baseline --------- Co-authored-by: NianJiuZst <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>
…hreadDirectMessages The 7.2 cherry-pick resolved extensions/mattermost/src/mattermost/monitor.ts by taking our side wholesale, silently reverting six upstream commits landed between 7.1 and 7.2. Adopt them instead of clobbering: f77c2aa truncate the inbound preview on a code-point boundary (openclaw#101630) b7ab62c preserve text-block boundaries in the draft preview (openclaw#87449) da6a24d prevent hung pre-delivery hooks from blocking lanes (openclaw#104256) 218dcd8 noUncheckedIndexedAccess in the extensions lane (openclaw#105132) 9cdf166 trim the monitor gating facade (openclaw#106021) 70a7e6e (opt-in DM threading by chat type) was already in the impl; three 7.1-era assertions in monitor.test.ts that upstream had superseded move to upstream's semantics (the implementations are byte-identical to upstream). b7ab62c's 173-line monitor hunk (block-preview boundary machinery) was hand-ported onto our activity/warm-fallback interleaving; the block-preview code is now byte-identical to pristine 7.2. threadDirectMessages kept as a thin compat feeding upstream's replyToModeByChatType, because Dockerfile.base's shared config probe still writes it and the probe spans matrix bases older than 2026.7.2. Retire both together later. Mattermost suite 630/632. The 2 remaining timeouts are localized to deliverMattermostReplyWithDraftPreview: with a mocked-rejecting updateMattermostPost, our patched buildFinalEdit attempts the in-place edit (pristine skips it), the edit rejects, and the deliverNormally fallback awaits the unmocked deliverMattermostReplyPayload which never resolves under the test's bare sendMessageMattermost stub. Behavioral divergence in our activity/warm-fallback finalize path, not the upstream port. TODO.
Closes #103684.
What Problem This Solves
Fixes an issue where a final channel reply could remain undelivered and keep the serialized reply lane busy forever when a pre-delivery callback never settled. Affected users could see reactions but no answer, while later messages accumulated behind the stuck lane.
The original report was captured on WhatsApp 2026.6.11. The exact callback was not isolated, so this change covers every remaining production pre-delivery owner without changing the foreground freshness contract.
Why This Change Was Made
The dispatcher keeps one flattened internal stage list. Constructor, channel-custom, and post-construction stages each receive their own 15-second settlement deadline; appended stages cannot escape the policy, and two individually valid slow stages do not share an aggregate deadline. The foreground freshness fence remains lifecycle-owned and unbounded because timing it out could release a stale older reply.
message_sendingandreply_payload_sendinguse the hook runner's existing 15-second per-handler default and fail-open behavior. Explicit plugin/operator hook timeout overrides remain authoritative.The shipped
openclaw/plugin-sdk/reply-runtimedispatcher contract now also lets channel-plugin owners declare a positive finite budget for their own callback:beforeDeliverOptions: { timeoutMs }at construction, ordispatcher.appendBeforeDeliver(handler, { timeoutMs })for appended work. The default remains 15 seconds, while an explicit budget is still per stage. This adds no config key, environment variable, protocol field, or general SDK configuration surface.A timeout is recorded as
failed-before-deliver, not as an intentional cancellation. Per-payload completion preservespendingFinalDeliveryonly when every relevant final failed before transport started; delivery, explicit suppression, or an error after transport began retains the existing no-blind-retry behavior. The publicsend*(): booleancontracts are unchanged.The one bundled Mattermost button-click caller that skipped dispatcher settlement now uses the existing shared lifecycle owner; all other bundled/core callers were already settled. There is no persistent data-model or migration change: the documentation update describes the public callback contract only.
User Impact
A hung pre-delivery callback releases the owning lane after a bounded wait, so subsequent replies can continue. Plugin hook timeouts continue with the latest safe payload. Channel plugins with legitimate slower
beforeDeliverwork can declare a larger per-callback deadline instead of being forced into the new 15-second default. A final that definitely failed before send keeps its durable pending state instead of being silently marked delivered.Evidence
Deterministic repository regressions cover:
Completed with the exact pushed content (
625b270e6fff00a0958a9f402791352e93dda6be):node scripts/run-vitest.mjs src/auto-reply/reply/reply-flow.test.ts src/auto-reply/reply/dispatch-from-config.reply-dispatch.test.ts src/auto-reply/dispatch.freshness.test.ts src/plugins/wired-hooks-message.test.ts src/plugins/wired-hooks-reply-payload-sending.test.ts src/plugins/contracts/plugin-sdk-subpaths.test.ts— passed: 6 files / 82 tests across 3 Vitest shards.node scripts/build-all.mjs— passed.node scripts/package-openclaw-for-docker.mjs --allow-unreleased-changelog --skip-build --output-dir <temp> --output-name openclaw-current.tgz— passed the package inventory and tarball import-graph checks.openclaw/plugin-sdk/reply-runtimeresolved to the installed package'sdist/plugin-sdk/reply-runtime.js.beforeDeliver timed out after 15000ms; the same dispatcher deliveredfollow-up-final, with failed counts{ tool: 0, block: 0, final: 1 }.budget-final:constructor:appendedwith no errors and final failed count0.node --max-old-space-size=8192 --import tsx scripts/generate-plugin-sdk-api-baseline.ts --check— passed after refreshing the committed SDK baseline hash.OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs <touched TypeScript files>andgit diff --check— passed..agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --stream-engine-output— no accepted/actionable findings reported.Runtime proof boundary:
maincheckout, with public networking and no hydration. It could not acquire broker credentials because this environment has no EC2 IMDS role; no credentialed Testbox or live-channel run was used.