Skip to content

Harden ready-batch dispatch and process-exit cleanup - #1666

Open
minggangw wants to merge 7 commits into
RobotWebTools:developfrom
minggangw:fix/executor-ready-batch
Open

minggangw wants to merge 7 commits into
RobotWebTools:developfrom
minggangw:fix/executor-ready-batch

Conversation

@minggangw

@minggangw minggangw commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

Summary

Keep callback dispatch fail-fast while preventing stale entity access,
ensuring exception-safe resource cleanup, and fixing the exit-time crash
exposed by ASan/LSan.

Changes

  • Invalidate ready batches when nodes stop or are destroyed, including immediate stop/restart sequences.
  • Skip destroyed event handlers and recheck action entities between successive ready events.
  • Release raw ROS messages, action-expiration buffers, and retired handle references through finally blocks.
  • Document callback exception behavior for spin() and spinOnce().
  • Shut down all live contexts on process exit before native library teardown.
  • Add regression coverage for callback errors, entity destruction, stop/restart, and default/custom context cleanup across exit paths.
  • Include child-process termination details and captured output in unhandled-callback test failures.

Fix: #1667

Copilot AI balanced review requested due to automatic review settings October 27, 2026 09:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Continued dispatch can still access released event handlers because the ready-event loop does not recheck their destruction state.

1 open issue
What changed in this PR

This PR improves executor error recovery by preserving valid ready callbacks after failures while preventing stale batches from continuing after node shutdown or restart.

Changes:

  • Collect callback errors and report them after processing the valid batch.
  • Invalidate stopped batches, recheck action lifetimes, and release temporary native resources through finally blocks.
  • Document error reporting and add regression tests for recovery, lifecycle boundaries, and cleanup.
File Description
types/​node.d.ts Documents batch callback error reporting.
test/​test-spinning.js Adds error recovery, lifecycle, and resource cleanup tests.
lib/​node.js Implements guarded dispatch, error aggregation, and cleanup.

Review effort: Balanced

Comment thread lib/node.js Outdated
@coveralls

coveralls commented Oct 27, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 91.177% (+0.01%) from 91.164% — minggangw:fix/executor-ready-batch into RobotWebTools:develop

@minggangw minggangw changed the title Preserve Ready Callbacks Across Errors and Invalidate Stopped Batches Harden Callback Lifetimes and Exception-Path Cleanup Oct 28, 2026
@minggangw
minggangw requested a balanced review from Copilot October 28, 2026 05:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The regressions do not cover expiration-processing failures, leaving unverified that GoalInfoArray.freeArray runs exactly once and the original exception propagates.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment thread lib/node.js
Comment on lines +523 to +524
} finally {
GoalInfoArray.freeArray(message);
@minggangw
minggangw requested a balanced review from Copilot October 28, 2026 07:47
@minggangw minggangw changed the title Harden Callback Lifetimes and Exception-Path Cleanup Harden ready-batch dispatch and process-exit cleanup Oct 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Exception-path regression coverage is missing for expired-goal cleanup, including verification that failures in actionExpireGoals and processGoalExpired call freeArray exactly once and propagate the original exception.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The exit hook shuts down live contexts before destruction, causing DestroyContext() to skip finalization and leak native allocations.

2 open issues

Review effort: Balanced

Comment thread index.js
rcl.shutdownAll();
};
process.on('SIGINT', _sigHandler);
process.once('exit', () => rcl.shutdownAll());

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The non-removable exit listener shuts down contexts before application exit handlers, causing final synchronous publishes to use released publisher handles and leaving no supported opt-out.

3 open issues

Review effort: Max

Comment thread index.js
rcl.shutdownAll();
};
process.on('SIGINT', _sigHandler);
process.once('exit', () => rcl.shutdownAll());

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ready-batch dispatch continues after node stop and can access destroyed entities

3 participants