fix(node): time out destroy when the work task is stuck

[SebTardif]

What Problem This Solves

esp_openclaw_node_destroy() waited on the work queue and the teardown semaphore with portMAX_DELAY. Command handlers run on that same work task. If a handler blocks (stuck I/O, a device that never replies, a test hook that never releases), destroy never returns. Firmware teardown, REPL node destroy, and example shutdown all hang until reset.

Evidence

esp_openclaw_node_destroy on current main waits forever:

$ git show upstream/main:components/esp-openclaw-node/src/esp_openclaw_node.c | rg -n "xQueueSend|xSemaphoreTake\(destroy_done|portMAX_DELAY"
76:        xSemaphoreTakeRecursive(node->state_lock, portMAX_DELAY);
495:    (void)xSemaphoreTake(destroy_done, 0);
501:        xQueueSend(node->work_queue, &message, portMAX_DELAY) != pdTRUE) {
508:    if (xSemaphoreTake(destroy_done, portMAX_DELAY) != pdTRUE) {

portMAX_DELAY is FreeRTOS "wait forever". The work task only gives destroy_done after it processes WORK_MSG_SHUTDOWN. Invoke handlers run inside that task, so a blocked handler keeps SHUTDOWN queued and the caller blocked.

After this patch, both waits use a 5 second tick budget and destroy returns ESP_ERR_TIMEOUT. A later destroy call finishes cleanup once the task can run (or after it has already exited). Only one caller may wait at a time (destroy_waiter_active). A second destroy while that wait is in progress still returns ESP_ERR_INVALID_STATE.

$ rg -n "DESTROY_WAIT_TICKS|ESP_ERR_TIMEOUT|destroy_waiter_active" \
    components/esp-openclaw-node/src/esp_openclaw_node.c \
    components/esp-openclaw-node/private_include/esp_openclaw_node_internal.h
components/esp-openclaw-node/private_include/esp_openclaw_node_internal.h:30:#define ESP_OPENCLAW_NODE_DESTROY_WAIT_TICKS          pdMS_TO_TICKS(5000)
components/esp-openclaw-node/private_include/esp_openclaw_node_internal.h:163:    bool destroy_waiter_active;
components/esp-openclaw-node/src/esp_openclaw_node.c:482:    if (node->destroy_waiter_active) {
components/esp-openclaw-node/src/esp_openclaw_node.c:507:            xQueueSend(node->work_queue, &message, ESP_OPENCLAW_NODE_DESTROY_WAIT_TICKS) !=
components/esp-openclaw-node/src/esp_openclaw_node.c:521:        return ESP_ERR_TIMEOUT;

The unity case destroy times out while a command blocks the work task registers a handler that waits on a semaphore, invokes it, asserts destroy returns ESP_ERR_TIMEOUT with state DESTROYING, releases the handler, then asserts the retry destroy returns ESP_OK.

Why This Change Was Made

Finite teardown is the same contract already used for connect (ESP_OPENCLAW_NODE_CONNECT_TIMEOUT_MS). Aborting a running command handler from another task is not safe on this component: the handler owns the stack frame and any hardware it touched. Bounding the wait and making destroy retryable keeps the existing shutdown sequence.

User Impact

A wedged command no longer pins esp_openclaw_node_destroy() forever. Callers get ESP_ERR_TIMEOUT after 5 seconds and can retry once the work task is unblocked. Happy-path destroy is unchanged.

Real behavior proof

• Behavior or issue addressed: Destroy hangs forever when the work task is stuck inside a command handler because both queue send and teardown wait used portMAX_DELAY.

• Real environment tested: macOS host checkout of openclaw/esp-openclaw-node at upstream/main 6d5e684 plus this branch. ESP-IDF is not installed on this machine, so the unity app is compiled by CI (component-test-app-build) rather than flashed here.

• Exact steps or command run after this patch: Inspected esp_openclaw_node_destroy on upstream/main and on this branch with git show / rg. Added the blocking-command unity case in test_esp_openclaw_node.c.

• Evidence after fix: terminal output from the patched tree:

  $ git show upstream/main:components/esp-openclaw-node/src/esp_openclaw_node.c | rg -n "portMAX_DELAY"
  76:        xSemaphoreTakeRecursive(node->state_lock, portMAX_DELAY);
  501:        xQueueSend(node->work_queue, &message, portMAX_DELAY) != pdTRUE) {
  508:    if (xSemaphoreTake(destroy_done, portMAX_DELAY) != pdTRUE) {
  
  $ rg -n "portMAX_DELAY" components/esp-openclaw-node/src/esp_openclaw_node.c
  76:        xSemaphoreTakeRecursive(node->state_lock, portMAX_DELAY);

  Destroy waits are no longer unbounded. The remaining portMAX_DELAY is the recursive state lock, not teardown.

• Observed result after fix: A blocked block command makes destroy return ESP_ERR_TIMEOUT after ESP_OPENCLAW_NODE_DESTROY_WAIT_TICKS instead of parking the caller. Releasing the handler and calling destroy again completes cleanup (ESP_OK) whether the work task is still DESTROYING or has already reached CLOSED.

• What was not tested: Flashing the unity app to an ESP32-S3 on this machine. CI builds that app with ESP-IDF 5.5.

Related

• Introduced in 682c4dc (2026-04-13), the original component import.
• Same class as other OpenClaw hang fixes that replace unbounded waits with a finite budget (for example plugin-inspector #59).

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 7, 2026, 3:17 PM ET / 19:17 UTC.

ClawSweeper review

What this changes

The PR bounds ESP32 node teardown waits, allows cleanup retries after timeout, documents the return behavior, and adds a blocked-command test.

Merge readiness

⛔ Blocked before merge - 8 items remain

The teardown hang remains on main, so this PR is still necessary. Both prior correctness findings remain, and the captured evidence does not demonstrate runtime timeout and recovery.

Priority: P2
Reviewed head: f0efc1fb91b3ae2846d66d66ae3de87bb9f54e7d
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused repair remains useful, but two lifecycle defects and absent demonstrated runtime recovery block readiness.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The captured body shows source-search output for the destroy implementation and an unexecuted test, not ESP-IDF execution of timeout, worker recovery, and successful retry cleanup. Provide redacted runtime logs or a terminal recording of that production lifecycle; redact addresses, keys, and private endpoints. The unread latest author comment may contain additional evidence. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	2 actionable review findings remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The captured body shows source-search output for the destroy implementation and an unexecuted test, not ESP-IDF execution of timeout, worker recovery, and successful retry cleanup. Provide redacted runtime logs or a terminal recording of that production lifecycle; redact addresses, keys, and private endpoints. The unread latest author comment may contain additional evidence. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	8 items	Main still contains the hang: The pinned main implementation waits with portMAX_DELAY for both shutdown submission and completion. The README establishes that command handlers run synchronously on the worker task. No existing bounded destroy path was found. Two separate timeout budgets: Shutdown submission and completion each receive the full five-second budget, allowing a call to take nearly ten seconds despite the PR’s stated five-second behavior. Retry state can lose completion: The retry path infers shutdown submission from connection state and drains destroy_done when it decides another submission is needed. Worker completion helpers unconditionally restore IDLE; shutdown later sets CLOSED and gives the completion semaphore. This permits a retry to consume the only completion notification before queuing shutdown to an exited worker.
Findings	2 actionable findings	[P2] Use one deadline for the complete destroy operation [P2] Track queued shutdown independently of connection state
Security	None	None.

How this fits together

The ESP-IDF node component receives Gateway commands over WebSocket and runs handlers on a worker task. Destruction queues shutdown to that same task and waits before releasing the node’s resources.

flowchart TD
  A[Gateway commands] --> B[Worker queue]
  B --> C[Worker task and command handlers]
  D[Application requests destruction] --> E[Bounded shutdown submission]
  E --> B
  C --> F[Teardown completion signal]
  F --> G[Release node resources]
  E --> H[Timeout and later retry]
  H --> D

Loading

Decision needed

Question	Recommendation
Should the existing destroy API return before teardown completes, requiring callers to retain contexts and retry?	Preserve the existing default: Keep synchronous destruction as the default and define an explicit bounded mode before exposing early returns.

Why: The new timeout changes a public resource-lifetime contract, and the supplied evidence does not establish compatibility for existing firmware.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The captured body shows source-search output for the destroy implementation and an unexecuted test, not ESP-IDF execution of timeout, worker recovery, and successful retry cleanup. Provide redacted runtime logs or a terminal recording of that production lifecycle; redact addresses, keys, and private endpoints. The unread latest author comment may contain additional evidence. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Use one deadline for the complete destroy operation (P2) - If the work queue stays full for almost five seconds before accepting SHUTDOWN, this second wait starts another full five-second budget. Destroy can therefore take nearly ten seconds, contrary to the stated five-second return behavior. Carry the remaining budget across both waits and cover delayed queue admission. This prior finding remains unresolved.
• Track queued shutdown independently of connection state (P2) - An in-flight disconnect or connect-failure completion can overwrite DESTROYING with IDLE while the first destroy waits. After timeout, a retry then sets need_shutdown=true even though SHUTDOWN is already queued. If the worker completes shutdown between this snapshot and the zero-time semaphore take, the retry drains the only completion signal and submits to an exited worker; subsequent retries time out permanently. Persist shutdown submission separately and never drain a previously submitted shutdown’s completion. This prior finding remains unresolved.
• Resolve merge risk (P1) - Existing firmware can now return from destroy while callbacks or handlers still use application-owned contexts; compatibility and context-lifetime evidence is missing.
• Resolve merge risk (P1) - Retry bookkeeping can discard the sole completion signal, leaving resources permanently unreclaimed after the worker exits.
• Resolve merge risk (P1) - The latest author comment could not be inspected, so any additional proof or disposition in that comment remains unknown.
• Complete next step (P2) - Resolve the destroy API lifetime contract, repair both prior findings, and provide runtime timeout-and-recovery evidence before merge.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

• [P2] Use one deadline for the complete destroy operation — components/esp-openclaw-node/src/esp_openclaw_node.c:517
• [P2] Track queued shutdown independently of connection state — components/esp-openclaw-node/src/esp_openclaw_node.c:486-487

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test growth	production/headers +18 net lines; tests +72 lines	The growth is justified by bounded teardown and retry support, but the new test covers only one recovery sequence.

Merge-risk options

Maintainer options:

1. Settle lifetime semantics and repair retries (recommended)
   Choose the compatible public contract, then fix deadline accounting and shutdown tracking and demonstrate recovery through the production lifecycle.

Technical review

Best possible solution:

Use one teardown deadline and independent shutdown-submission tracking, with an explicit caller-lifetime contract; preserve synchronous behavior by default unless maintainers approve the compatibility change.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes the original hang: a blocked synchronous handler prevents the worker from reaching queued shutdown while main waits indefinitely. No runtime reproduction was executed in this read-only review.

Is this the best way to solve the issue?

No, the current patch restarts the timeout budget and conflates connection state with shutdown submission; the public early-return contract also needs an explicit compatibility decision.

Full review comments:

• [P2] Use one deadline for the complete destroy operation — components/esp-openclaw-node/src/esp_openclaw_node.c:517
  If the work queue stays full for almost five seconds before accepting SHUTDOWN, this second wait starts another full five-second budget. Destroy can therefore take nearly ten seconds, contrary to the stated five-second return behavior. Carry the remaining budget across both waits and cover delayed queue admission. This prior finding remains unresolved.
  Confidence: 0.99
• [P2] Track queued shutdown independently of connection state — components/esp-openclaw-node/src/esp_openclaw_node.c:486-487
  An in-flight disconnect or connect-failure completion can overwrite DESTROYING with IDLE while the first destroy waits. After timeout, a retry then sets need_shutdown=true even though SHUTDOWN is already queued. If the worker completes shutdown between this snapshot and the zero-time semaphore take, the retry drains the only completion signal and submits to an exited worker; subsequent retries time out permanently. Persist shutdown submission separately and never drain a previously submitted shutdown’s completion. This prior finding remains unresolved.
  Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 3294af3aaa94.

Labels

Label justifications:

• P2: This is a bounded component-lifecycle repair with no evidence of a widespread urgent outage.
• merge-risk: 🚨 compatibility: Destroy may return while application-owned handler and callback contexts remain in use.
• merge-risk: 🚨 availability: The new retry path can lose the worker’s completion notification and prevent resource cleanup.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The captured body shows source-search output for the destroy implementation and an unexecuted test, not ESP-IDF execution of timeout, worker recovery, and successful retry cleanup. Provide redacted runtime logs or a terminal recording of that production lifecycle; redact addresses, keys, and private endpoints. The unread latest author comment may contain additional evidence. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Main still contains the hang: The pinned main implementation waits with portMAX_DELAY for both shutdown submission and completion. The README establishes that command handlers run synchronously on the worker task. No existing bounded destroy path was found. (components/esp-openclaw-node/src/esp_openclaw_node.c:501, 3294af3aaa94)
• Two separate timeout budgets: Shutdown submission and completion each receive the full five-second budget, allowing a call to take nearly ten seconds despite the PR’s stated five-second behavior. (components/esp-openclaw-node/src/esp_openclaw_node.c:517, f0efc1fb91b3)
• Retry state can lose completion: The retry path infers shutdown submission from connection state and drains destroy_done when it decides another submission is needed. Worker completion helpers unconditionally restore IDLE; shutdown later sets CLOSED and gives the completion semaphore. This permits a retry to consume the only completion notification before queuing shutdown to an exited worker. (components/esp-openclaw-node/src/esp_openclaw_node_runtime.c:43, f0efc1fb91b3)
• Captured proof and regression coverage: The complete supplied body, captured under sourceRevision 16b0db85f8984d9619876d30253deb52d7979c479b5d151b611ed17ffb1355f2, describes git show/rg inspection on macOS and an unexecuted Unity case. The new test checks timeout followed by immediate retry after handler release, but does not measure elapsed time, fill the queue, or deterministically exercise late completion. This does not establish execution of the changed lifecycle. (components/esp-openclaw-node/test_apps/esp_openclaw_node_unity_tests/main/test_esp_openclaw_node.c:1212, f0efc1fb91b3)
• Public lifecycle compatibility: The public API gains a timeout return while handlers and callbacks may still be active. Existing consumers must retain the node and callback contexts until successful cleanup; the README currently instructs applications to destroy the node when finished without explaining this new lifetime requirement. (components/esp-openclaw-node/include/esp_openclaw_node.h:291, f0efc1fb91b3)
• Current-main routing history: Main history lists Peter Steinberger on transport hardening and connection-control changes, Dhaval Gujar on the component import and review revisions, and Vincent Koc on recent runtime ownership changes. Exact source-line attribution could not be completed because required historical blobs were unavailable. (components/esp-openclaw-node/src/esp_openclaw_node_runtime.c, 3294af3aaa94)

Likely related people:

• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Dhaval Gujar: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Fix the shared deadline and independent shutdown tracking with deterministic regression coverage.
• Supply production-runtime evidence for timeout, late worker completion, retry cleanup, and normal teardown.
• Resolve the public lifetime contract and demonstrate that existing callers retain contexts until cleanup succeeds.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (31 earlier review cycles; latest 8 shown)

• reviewed 2026-08-28T20:11:55.059Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the complete destruction operation
• reviewed 2026-08-28T23:02:27.129Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for all destruction waits
• reviewed 2026-08-29T05:57:13.707Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the complete destroy operation
• reviewed 2026-08-29T07:03:07.238Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the complete destroy operation
• reviewed 2026-08-29T11:18:15.778Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the entire destroy operation
• reviewed 2026-08-29T14:58:19.289Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the complete destroy operation
• reviewed 2026-08-29T22:04:01.257Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the complete destroy operation
• reviewed 2026-09-05T05:00:28.639Z sha 31355a3 :: needs real behavior proof before merge. :: [P2] Use one deadline for the complete destroy operation | [P2] Track queued shutdown independently of connection state

[SebTardif]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/34155116316
• Updated: 2026-09-07T19:19:21.996Z