Skip to content

fix(launch_manager): remove allocation in ProcessGroupManager getters… - #699

Merged
NicolasFussberger merged 7 commits into
eclipse-score:mainfrom
Valeo-S-CORE-Organization:fix/remove-pgm-allocations
Oct 19, 2026
Merged

NicolasFussberger merged 7 commits into
eclipse-score:mainfrom
Valeo-S-CORE-Organization:fix/remove-pgm-allocations

Conversation

@MohamedRady-99

Copy link
Copy Markdown
Contributor

Description

  1. Introduce a lightweight, stack-allocated CompletionSlot<T> abstraction backed by score::concurrency::Notification and std::optional<T> inline storage, replacing heap-allocating InterruptiblePromise/InterruptibleFuture across getActiveRunTarget() and setRequestedRunTarget()
  2. Event representations (GetActiveRunTarget and SetRequestedRunTarget) now carry a non-owning pointer to the caller's stack-allocated slot instead of moving a promise into the event queue, ensuring zero dynamic memory allocations during runtime state queries and transitions.
  3. Also fixes a potential deadlock on shutdown: ComponentEventQueue::push() now correctly returns false on any enqueue failure (including ConcurrencyErrc::kStopped), and the getters/setters immediately return score::MakeUnexpected(ExecErrc::kFailed) rather than blocking indefinitely if the queue is stopped.
  4. Adds unit tests in completion_slot_UT verifying synchronous/asynchronous completion, stop-token aborts, and validating zero dynamic allocations via global allocation tracking for both Result<IdentifierHash> and Result<void>. Also adds concurrent execution and stopped-queue coverage in process_group_manager_UT and component_event_queue_UT.

Related issue

fixes #641

@paulquiring

paulquiring commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

CompletionSlot::wait() takes a score::cpp::stop_token (completion_slot.hpp), but the abstraction can't actually support cancellation safely, and I think we should remove the parameter rather than leave it available.

ComponentEvent only carries a non-owning raw pointer to the caller's stack-allocated CompletionSlot (GetActiveRunTarget::completion, SetRequestedRunTarget::completion). That's safe only as long as the caller unconditionally blocks until the consumer thread calls complete() on it — which is exactly what happens today, since both call sites in process_group_manager.cpp pass cpp::stop_token{} (a default-constructed, permanently non-abortable token: state_ is null, so stop_requested() is always false).

But the API signature invites the opposite: if a caller ever passes a real, abortable token (built from a stop_source, as PreCancelledStopTokenAbortsWait in completion_slot_UT.cpp already demonstrates in isolation) and stop is requested while the corresponding event is still sitting in the queue, wait() returns std::nullopt, the caller's stack frame unwinds, the CompletionSlot is destroyed — and then the consumer thread later dequeues the event and calls event->completion->complete(result) on a dangling pointer. That's a use-after-free, and nothing here catches it: Notification's internal entry_counter_ guard only protects against re-entering the same Notification instance concurrently, it does nothing once waitWithAbort has already returned on abort.

Since nothing in this PR actually needs cancellable waits (both call sites use the non-abortable default), I'd suggest dropping the stop_token parameter from wait() entirely so the type only allows the one usage pattern that's actually sound. If cancellable completion is a real future requirement, it'll need a design where the event can never call complete() after cancellation (e.g. an atomic pointer the consumer swaps-and-checks before dereferencing) rather than a bare raw pointer plus caller discipline.

cc @NicolasFussberger @MaciejKaszynski

@MohamedRady-99

Copy link
Copy Markdown
Contributor Author

Hi @paulquiring
Thank you for the review I have addressed this in the latest commit by enforcing unconditional waiting:

  • Removed stop_token from CompletionSlot::wait(): The method now blocks unconditionally until complete() executes, guaranteeing the caller’s stack frame remains valid throughout the event lifecycle.
  • Simplified the API return type: Since completion is guaranteed before wait() returns, the method now returns T directly rather than std::optional, eliminating unnecessary optional unwrapping at call sites.
  • Updated call sites in ProcessGroupManager: getActiveRunTarget() and setRequestedRunTarget() now directly return slot.wait().
  • Updated test suite: Removed the cancellation test case and adjusted completion_slot_UT to validate the revised signature.

All unit tests, including AddressSanitizer and ThreadSanitizer checks, continue to pass cleanly with zero dynamic memory allocations and no race conditions.

@NicolasFussberger

Copy link
Copy Markdown
Contributor

Apologies for the late response. Paul is currently unavailable to complete the review so I will take over the review.
Could you rebase your branch @MohamedRady-99 ?

@NicolasFussberger

Copy link
Copy Markdown
Contributor

@NicolasFussberger i reworded the message to fit the size limit

Thanks, but now github reports conflicts that prevent me from merging.

This branch has conflicts that must be resolved

Signed-off-by: MohamedRady-99 <mohamed.hossam-rady@valeo.com>
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 13, 2026 08:35 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 13, 2026 08:35 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 13, 2026 08:35 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 13, 2026 08:35 — with GitHub Actions Active
@MohamedRady-99

Copy link
Copy Markdown
Contributor Author

@NicolasFussberger i reworded the message to fit the size limit

Thanks, but now github reports conflicts that prevent me from merging.

This branch has conflicts that must be resolved

conflicts solved

@NicolasFussberger

Copy link
Copy Markdown
Contributor

@MohamedRady-99 sorry for the delay. There were central issues with the pipelines in score which prevented merging your PR. Could you kindly rebase one last time? Thank you.

@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 07:27 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 07:27 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 07:27 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 07:27 — with GitHub Actions Active
@MohamedRady-99

Copy link
Copy Markdown
Contributor Author

@MohamedRady-99 sorry for the delay. There were central issues with the pipelines in score which prevented merging your PR. Could you kindly rebase one last time? Thank you.

I’ve rebased the branch, and it’s now ready to be merged.

@MohamedRady-99
MohamedRady-99 force-pushed the fix/remove-pgm-allocations branch from c99b657 to ac37c4a Compare October 19, 2026 08:17
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 08:18 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 08:18 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 08:18 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 08:18 — with GitHub Actions Active
@NicolasFussberger

NicolasFussberger commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Now formatting pre commit check fails, please run bazel run //:format.fix

@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 10:10 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 10:10 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 10:10 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 deployed to workflow-approval October 19, 2026 10:10 — with GitHub Actions Active
@MohamedRady-99
MohamedRady-99 force-pushed the fix/remove-pgm-allocations branch from 86f1727 to 9719e29 Compare October 19, 2026 10:42
@github-actions

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-699: https://eclipse-score.github.io/lifecycle/pr-699/

This branch was successfully deployed

1 active deployment
workflow-approval — 7fc8d7ef Deployed Oct 19, 2026 by MohamedRady-99 via Build and test unit-tests-arm64-qnx / approval #1570
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Remove allocation in ProcessGroupManager getters/setters

3 participants