Skip to content

relay: release a cross-exec subgroup's consumers on its target executor - #793

Open
afrind wants to merge 1 commit into
mainfrom
fix/cross-exec-subgroup-release
Open

afrind wants to merge 1 commit into
mainfrom
fix/cross-exec-subgroup-release

Conversation

@afrind

@afrind afrind commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

The publisher's session drops its ref to a cross-exec subgroup filter on its own io thread right after endOfSubgroup(). When the relay runs the queued endOfSubgroup first, that drop destroys the filter on the io thread, and ~SubgroupWriteback unpins its group and pushes onto the cache LRU while the relay thread edits the same list. The corrupted list later crashes in pinGroup and evictGroup.

The destructor now posts the release of downstream_ and keepAlive_ to targetExec_, as FetchCrossExecFilter already does.

Fixes: #787
Fixes: #789


This change is Reviewable

Summary by CodeRabbit

  • Bug Fixes
    • Subgroup consumers and related resources are now released on the designated executor, helping ensure cleanup occurs on the expected thread.

The publisher's session drops its ref to a cross-exec subgroup filter on its
own io thread right after endOfSubgroup(). When the relay runs the queued
endOfSubgroup first, that drop destroys the filter on the io thread, and
~SubgroupWriteback unpins its group and pushes onto the cache LRU while the
relay thread edits the same list. The corrupted list later crashes in
pinGroup and evictGroup.

The destructor now posts the release of downstream_ and keepAlive_ to
targetExec_, as FetchCrossExecFilter already does.

Fixes: #787
Fixes: #789

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: openmoq/moqx/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8a955597-c01d-4cd9-952d-03b9dc4fa894
📥 Commits

Reviewing files that changed from the base of the PR and between c683d9a and 2606453.

📒 Files selected for processing (2)
  • src/relay/CrossExecFilter.h
  • test/CrossExecFilterTest.cpp

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

CrossExecSubgroupFilter schedules destruction of its downstream_ and keepAlive_ references on targetExec_. A new test checks that the inner subgroup is destroyed on the relay executor.

Changes

Subgroup lifetime

Layer / File(s) Summary
Target-executor release
src/relay/CrossExecFilter.h, test/CrossExecFilterTest.cpp
The filter destructor schedules release of downstream_ before keepAlive_ on targetExec_. The test checks that the inner subgroup is destroyed on the relay executor after the session’s remaining reference is released on another thread.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 26064

The change addresses the reported subgroup teardown crash, and no concrete remaining issue is established. Normal validation remains appropriate before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 26064

The change improves cleanup isolation and preserves the subgroup’s parent during release. No new external access or privilege is identified. The remaining uncertainty is whether deferred cleanup always finishes before its executor is destroyed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The reviewed cleanup behavior participates in existing publisher-to-relay and publisher-to-subscriber-executor forwarding paths. A lifetime violation would affect in-process forwarding or shared cache availability. No evidence establishes a new independently attackable tenant, credential, or privilege boundary.

Trust Boundaries and Controls

  • observed — The changed boundary is executor-owned mutable state, not an authentication or identity transition. Existing setup forwards subgroup identifiers, priority, and options to the downstream consumer. The added cleanup task captures existing references and makes no new authorization decision.

Resilience and Maintainability Implications

  • inferred — Explicit downstream-before-parent release improves ownership discipline on a live executor. Shutdown barriers and IO workers configured to wait for idle are meaningful counterevidence to premature teardown, but do not complete the missing owner-to-final-release proof for every subgroup.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issues [#787] and [#789] report relay crashes caused by cache-LRU corruption. CrossExecSubgroupFilter now moves downstream_ and keepAlive_ into a task on targetExec_, releasing downstream_ f…
Out of Scope Changes check ✅ Passed The destructor change and regression test both address the executor-affinity failure linked to [#787] and [#789]. The reviewed diff contains no unrelated changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: releasing a cross-exec subgroup’s consumers on its target executor.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

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.

Relay crash: SIGSEGV in std::__detail::_List_node_base::_M_unhook Relay crash: SIGSEGV in operator delete

1 participant