concurrency: fix placement new over the live value_ member

[kutsibalci]

Fixes #301.

What

InterruptibleState overwrites its live value_ member with placement new. Placement new ends the previous object's lifetime by reusing its storage ([basic.life]/1.5), but it never runs that object's destructor, so anything a non-trivially destructible score::Result holds is silently never released. The result of the placement new is also discarded. This fixes both, at all six occurrences.

+        // Placement-new ends the old object's lifetime by reusing its storage, but never runs its
+        // destructor; destroy explicitly so a non-trivially destructible score::Result releases what it holds.
+        std::destroy_at(&value_);
         // NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
-        new (&value_) score::Result<Value>{std::move(value)};
+        static_cast<void>(::new (&value_) score::Result<Value>{std::move(value)});

Why this shape of fix

The issue proposes value_ = score::Result<Value>{...} as preferred. That would change which types the class supports, so it is not used here. The comment above each call already records the reason:

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable

Verified against main:

static_assert(!std::is_move_assignable<score::Result<NonAssignable>>::value, "");
static_assert(std::is_move_constructible<score::Result<NonAssignable>>::value, "");

Both hold, so InterruptibleState<NonAssignable> compiles today and would stop compiling under the assignment form. Keeping construction and destroying the previous object explicitly fixes the defect without narrowing support.

Both idioms used here already exist in this repository: std::destroy_at in score/language/futurecpp/include/score/circular_buffer.hpp:102, and static_cast<void>(::new ...) for the deliberately discarded result in score/language/futurecpp/include/score/expected.hpp.

Scope: six occurrences, not three

The issue lists three and invites checking for others. A sweep of score/ found three more:

location	specialization
interruptible_state.h:83, 103, 119	InterruptibleState<Value> (listed in the issue)
interruptible_state.h:207, 223	InterruptibleState<Value&>
interruptible_state.cpp:36	InterruptibleState<void>

All six overwrite a live value_ member (declared at :176, :277, :308).

The other 49 placement-new uses in score/ are on raw storage with matching destruction and are not touched: variant.hpp, expected.hpp, inplace_vector.hpp, circular_buffer.hpp, memory_resource.cpp, move_only_function.hpp, chunk_list.hpp, free_list.hpp, construct_at.hpp, and score/os/static_destruction_guard.h:71 (refcount-guarded with a matching destructor).

Severity

Stated plainly so this is prioritised correctly: TestAndMarkValueAsSet() means these run once, and at that point value_ always holds the error alternative rather than a Value. score::result::Error is currently trivially destructible, so there is no leak today — as the issue itself notes. This is a latent defect rather than a live leak: the code silently depends on score::result::Error staying trivially destructible, and a destructor that has to run is skipped the moment that stops being true. Note that skipping it is not automatically undefined behaviour — [basic.life]/4 makes it undefined only for a program that depends on the destructor's side effects.

Verification

I could not run the full Bazel suite locally: the ape and rules_diff dependencies are hosted on gitlab.arm.com, which does not resolve from my machine, so bazel test --config=bl-x86_64-linux //score/concurrency/... aborts during fetch. I am relying on CI for the real build and flagging that rather than implying I ran it.

What I did verify locally, with g++ -std=c++17 -fsyntax-only against the repository include paths:

• interruptible_state.cpp compiles clean, no warnings.
• Explicit instantiation of InterruptibleState<std::string> (non-trivially-destructible Value), InterruptibleState<NonAssignable> (non-assignable Value) and InterruptibleState<int&> (the reference_wrapper specialization), so every changed line is instantiated.
• The two static_asserts above.

AI disclosure

AI-assisted (Claude Code). The tool was used to sweep the repository for the pattern, apply the change across the six sites, and draft this description. The results were checked before opening: the three additional sites were confirmed by reading the declarations of value_ in each specialization, the 49 other placement-new uses were reviewed and excluded individually, and the compilation checks above were run. I have reviewed and understood the change and take responsibility for it.

[github-actions]

The created documentation from the pull request is available at: docu-html

[4og]

Hi @kutsibalci, thanks for contribution!

@sankurm, can you please review this PR?

[Copilot]

Pull request overview

Fixes placement-new handling in InterruptibleState by explicitly destroying existing results before reconstructing them.

Changes:

• Adds std::destroy_at at all six reconstruction sites.
• Explicitly discards placement-new return values.
• Adds required <memory> includes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
score/concurrency/future/interruptible_state.h	Updates value and reference specializations.
score/concurrency/future/interruptible_state.cpp	Updates the void specialization.

Suppressed comments (4)

score/concurrency/future/interruptible_state.h:107

• This explanation is inaccurate: placement construction itself ends the previous object's lifetime by reusing its storage, even when the type is non-trivially destructible. The explicit destruction is still needed to run the old object's destructor; describe that consequence instead of claiming the former code was automatically undefined behavior.

        // The existing object is destroyed first: reusing the storage of a live object without ending its
        // lifetime is undefined behaviour as soon as score::Result is not trivially destructible.

score/concurrency/future/interruptible_state.h:126

• This explanation is inaccurate: placement construction itself ends the previous object's lifetime by reusing its storage, even when the type is non-trivially destructible. The explicit destruction is still needed to run the old object's destructor; describe that consequence instead of claiming the former code was automatically undefined behavior.

        // The existing object is destroyed first: reusing the storage of a live object without ending its
        // lifetime is undefined behaviour as soon as score::Result is not trivially destructible.

score/concurrency/future/interruptible_state.h:217

• This explanation is inaccurate: placement construction itself ends the previous object's lifetime by reusing its storage, even when the type is non-trivially destructible. The explicit destruction is still needed to run the old object's destructor; describe that consequence instead of claiming the former code was automatically undefined behavior.

        // The existing object is destroyed first: reusing the storage of a live object without ending its
        // lifetime is undefined behaviour as soon as score::Result is not trivially destructible.

score/concurrency/future/interruptible_state.h:236

• This explanation is inaccurate: placement construction itself ends the previous object's lifetime by reusing its storage, even when the type is non-trivially destructible. The explicit destruction is still needed to run the old object's destructor; describe that consequence instead of claiming the former code was automatically undefined behavior.

        // The existing object is destroyed first: reusing the storage of a live object without ending its
        // lifetime is undefined behaviour as soon as score::Result is not trivially destructible.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[kutsibalci]

That is a fair correction and I have taken it. Placement new does end the previous object's lifetime, by reusing its storage ([basic.life]/1.5) — what it does not do is run that object's destructor, and [basic.life]/4 makes the omission undefined only for a program that depends on the destructor's side effects, not unconditionally. The old comment got both halves wrong.

Reworded at all six sites in d53f1d3, and the pull request description corrected in the same places:

// Placement-new ends the old object's lifetime by reusing its storage, but never runs its
// destructor; destroy explicitly so a non-trivially destructible score::Result releases what it holds.
std::destroy_at(&value_);

The generated code is unchanged; only the rationale moved. The fix itself still stands for the reason the description already gave — at these six call sites value_ is live, score::result::Error happens to be trivially destructible today so nothing leaks yet, and the std::destroy_at is what keeps that from becoming a real leak if it ever stops being true.

AI disclosure: AI-assisted (Claude Code). I checked the cited standard wording before accepting the correction and take responsibility for it.

[kutsibalci]

#517 has merged, so the baseline this PR was waiting on is now on main.

I re-ran the merged characterization suite against this change. main at c44eca5a, tests
exactly as merged:

build	result
main as it stands	63/63
main + this PR's six std::destroy_at	63/63

So the change preserves every setter behaviour those tests describe.

What those tests do not show, which bears on whether this is worth your time

They show the change breaks nothing. They do not show it fixes anything observable — and having now
measured it, I do not think it does on today's code. I would rather say that than let you find it.

score::result::Error      trivially destructible: 1
score::Result<void>       trivially destructible: 1
score::Result<int>        trivially destructible: 1
score::Result<Tracked>    trivially destructible: 0     // Tracked holds a std::string

value_ is overwritten at most once per state. It is constructed holding the error alternative
(Error::kUnset), and TestAndMarkValueAsSet rejects every later call, so a second placement-new
never runs. The object each placement-new overwrites therefore always holds a score::result::Error
— and that is trivially destructible. Nothing is leaked today.

So the honest description of this PR is lifetime hygiene, not a bug fix:

• [basic.life]/1.5 does end the old object's lifetime when its storage is reused, but the destructor
  is not run — so the code is correct only for as long as the overwritten alternative stays
  trivially destructible. That is a property of score::result::Error and of the guard, neither of
  which this class states as a requirement.
• It also stops discarding the result of the placement-new.

Both are cheap, and both stop being free if the guard, the initial value, or the error type ever
changes.

If that makes it not worth merging, I would genuinely rather be told and close it. #301 could then
be closed as "correct as written", with this measurement as the reason — which is also a useful
outcome, and better than leaving the issue open on a hazard that does not currently exist.

CI is 25/25 green and the branch is mergeable.

AI disclosure: AI-assisted (Claude Code). I ran both builds and the trait probe above against
main before posting, and take responsibility for the measurements.

[sankurm]

This fix stays in the currently enforced constraints (std::enable_if_t) on the SetValue overloads. This is good. Thank you.