Skip to content

Commit b43e72a

Browse files
committed
fix(core): AggregateException composes its message unconditionally (#2309)
SR-AUD-098 clauses C2/C3/C4. AggregateException("outer", {a, b}) reported "outer" and DISCARDED the contained diagnostics; .NET composes unconditionally whenever there are inner exceptions. THE TICKET'S TWO BLOCKING GROUNDS ANSWER EACH OTHER. Ground 1 was representation: composition (C2) and preservation (C3/C4) cannot both hold under ONE stored string. The ticket measured all five candidate models -- closing C2 alone works, closing C3/C4 alone works, and closing BOTH yields "custom outer (a) (b) (a) (b)" and GROWS WITHOUT BOUND under repeated Flatten(). The only escape is a second std::string, which is exactly SA-3's case: a private data member on a public type with no vtable, mangled-symbol, signature or noexcept change. Taken. sizeof(AggregateException) 192 -> 224 and sizeof(UnobservedTaskExceptionEventArgs) -- a public header holding one by value -- 208 -> 240, both pinned by a layout test. Consumers must rebuild; no source change. Ground 2 was the reference text, and /rv settles it: if (_innerExceptions.Length == 0) return base.Message; sb.Append(base.Message); sb.Append(' '); for (...) { sb.Append('('); sb.Append(_innerExceptions[i].Message); sb.Append(") "); } sb.Length--; -- AggregateException.cs:339-360 Two details are transcribed rather than reconstructed. The composition is UNCONDITIONAL when there are leaves -- this port composed only for the DEFAULT message, which is the C2 defect and an inconsistency with itself. And the trailing `sb.Length--` means the string ends at ")" with no trailing space; that is reproduced by appending ") " and dropping the last character rather than by special-casing the final element, so the two spellings cannot drift apart. FLATTEN AND HANDLE DIFFER, AND THAT IS .NET'S DOING. Flatten: new AggregateException(GetType() == typeof(AggregateException) ? base.Message : Message, ...) :335 Handle: throw new AggregateException(Message, unhandled.ToArray(), ...) :281 Flatten passes the RAW message for a plain AggregateException and the COMPOSED one for a derived type -- and that discrimination is precisely what stops repeated flattening from accreting leaf text, which is the pathology the ticket measured. Handle passes the COMPOSED one, so a rethrown aggregate legitimately lists its leaves twice. The two are transcribed separately rather than sharing a helper that would hide the difference. typeid is the C++ counterpart of GetType(), and this class is polymorphic, so it reads the dynamic type as .NET's does. The composition runs in the CONSTRUCTOR rather than the getter. That is observationally identical rather than a shortcut: innerExceptions_ is fixed at construction and never mutated, here and in .NET, where _innerExceptions is a ReadOnlyCollection assigned once. Composing on access would additionally force getMessageProperty() to stop returning const std::string&, which is a public signature change this needs no part of. Three pins inverted, four cases added. Five mutations, all caught. ALSO REPAIRED: a pre-existing flake this ticket's gate run caught. StopwatchDefinedArithmeticTests.Fix2326_TheResolutionThatWasLost took 200 pairs of back-to-back timestamps and required the smallest positive delta to be under 100 units -- which measures the MACHINE, not the code. It passed eight times out of eight in isolation and failed once inside a full gate, where any of the 200 pairs can be preempted. The property is a UNIT, so it is now asserted against a unit: GetTimestamp() samples steady_clock's own epoch, so a division by 100 would put its value two orders of magnitude below the same clock read directly. A comparison of values, immune to scheduling -- and it still catches the mutation that restores the division. A test that is intermittently green is not evidence (#2352). Downstream: neither cna nor mobile-eggbert references AggregateException -- zero sites in both. The rebuild requirement is recorded for any future consumer. Gate: 17,333 run, 17,333 passed, 0 failed, 0 skipped across 38 executables, GREEN, confirmed by two consecutive full runs. docs/Migration-AggregateExceptionRawMessage.md
1 parent 839c101 commit b43e72a

6 files changed

Lines changed: 341 additions & 39 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `AggregateException` composes its message unconditionally (ticket #2309)
5+
6+
*2026-08-18.* `AggregateException("outer", {a, b}).what()` is now `"outer (a) (b)"`. It used to be
7+
`"outer"` — the contained diagnostics were **discarded** whenever the caller supplied a message.
8+
9+
`sizeof(AggregateException)` grows **192 → 224** and `sizeof(UnobservedTaskExceptionEventArgs)`
10+
**208 → 240**. **Downstream consumers must be recompiled**; no source change is needed.
11+
12+
Landed under `docs/StandingApprovals.md` **SA-3** (a private data member on a public type, with no
13+
vtable, mangled-symbol, signature or `noexcept` change, the before/after `sizeof` pinned by a
14+
layout test, and the full gate).
15+
16+
---
17+
18+
## 1. What changed
19+
20+
| Expression | Was | Is |
21+
|---|---|---|
22+
| `AggregateException("outer", {a, b})` | `"outer"` | `"outer (a) (b)"` |
23+
| `AggregateException("outer", a)` | `"outer"` | `"outer (a)"` |
24+
| `AggregateException({a, b})` | `"One or more errors occurred. (a) (b)"` | **unchanged** |
25+
| `AggregateException("bare")` | `"bare"` | **unchanged** — no leaves, no composition |
26+
| `outer.Flatten()` | `"One or more errors occurred. (a) (b)"` | `"outer (a) (b)"` — the caller's message survives |
27+
| `outer.Flatten().Flatten()` || `"outer (a) (b)"`**does not accrete** |
28+
| `Handle` rethrow || composed twice, deliberately — §4 |
29+
30+
## 2. Why the ticket was blocked, and what unblocked it
31+
32+
#2309 was blocked on two independent grounds.
33+
34+
**Ground 1 — representation.** Composition (C2) and preservation (C3/C4) cannot both hold under
35+
one stored string. The ticket measured all five candidate models: closing C2 alone works, closing
36+
C3/C4 alone works, and closing **both** yields `"custom outer (a) (b) (a) (b)"` and **grows
37+
without bound under repeated `Flatten()`**. The only escape is a second `std::string`.
38+
39+
That is exactly SA-3's case, so the field is taken.
40+
41+
**Ground 2 — reference text.** The composed grammar, and whether `Flatten`/`Handle` pass the raw
42+
or the composed message, were unverifiable. `/rv` settles both, and the answer is what makes the
43+
second field work.
44+
45+
## 3. The grammar
46+
47+
```csharp
48+
if (_innerExceptions.Length == 0) return base.Message;
49+
sb.Append(base.Message); sb.Append(' ');
50+
for (…) { sb.Append('('); sb.Append(_innerExceptions[i].Message); sb.Append(") "); }
51+
sb.Length--; // AggregateException.cs:339-360
52+
```
53+
54+
Two details are transcribed rather than reconstructed. The composition is **unconditional** when
55+
there are leaves — this port composed only for the *default* message, which is the C2 defect. And
56+
the trailing `sb.Length--` means the string ends at `")"` with **no** trailing space; that is
57+
reproduced by appending `") "` and dropping the last character, rather than by special-casing the
58+
final element, so the two spellings cannot drift apart.
59+
60+
The composition happens in the constructor rather than in the getter, and that is observationally
61+
identical rather than a shortcut: `innerExceptions_` is fixed at construction and never mutated,
62+
here and in .NET, where `_innerExceptions` is a `ReadOnlyCollection` assigned once. Composing on
63+
access would additionally force `getMessageProperty()` to stop returning `const std::string&`
64+
a public signature change this needs no part of.
65+
66+
## 4. `Flatten` and `Handle` differ, and that is .NET's doing
67+
68+
```csharp
69+
// Flatten
70+
new AggregateException(GetType() == typeof(AggregateException) ? base.Message : Message, …) // :335
71+
// Handle
72+
throw new AggregateException(Message, unhandledExceptions.ToArray(), …) // :281
73+
```
74+
75+
`Flatten` passes the **raw** message for a plain `AggregateException` and the **composed** one for
76+
a derived type. That discrimination is precisely what stops repeated flattening from accreting
77+
leaf textthe pathology the ticket measured. `typeid` is the C++ counterpart of `GetType()`, and
78+
this class is polymorphic, so it reads the *dynamic* type as .NET's does.
79+
80+
`Handle` passes the **composed** message. So a rethrown aggregate's message contains the leaves
81+
twice: `"custom outer (a) (b) (a) (b)"`. That is .NET's behaviour, not an accident here, and the
82+
two are transcribed separately rather than sharing a helper that would hide the difference.
83+
84+
## 5. To migrate
85+
86+
Rebuild. If you assert on an `AggregateException`'s message and supplied your own, expect the
87+
leaves to follow it.
88+
89+
## 6. Evidence
90+
91+
| Mutation | Caught |
92+
|---|---|
93+
| `Flatten` passes the composed message (the unbounded-growth pathology) | ✅ (2 tests) |
94+
| `Handle` passes the raw message (Flatten's rule, not its own) | ✅ |
95+
| Compose only for the default message (the pre-#2309 C2 behaviour) | ✅ (4 tests) |
96+
| Keep the trailing space (drop the `pop_back`) | ✅ (4 tests) |
97+
| The separator loses its space | ✅ (4 tests) |
98+
99+
## 7. A pre-existing flake this ticket's gate run caught
100+
101+
`StopwatchDefinedArithmeticTests.Fix2326_TheResolutionThatWasLost` took 200 pairs of back-to-back
102+
timestamps and required the smallest positive delta to be under 100 units. **That measures the
103+
machine, not the code**: it passed eight times out of eight in isolation and failed once inside a
104+
full gate, where any of the 200 pairs can be preempted.
105+
106+
The property is a **unit**, so it is now asserted against a unit: `GetTimestamp()` samples
107+
`steady_clock`'s own epoch, so a division by 100 would put its value two orders of magnitude below
108+
the same clock read directly. That is a comparison of values, immune to schedulingand it still
109+
catches the mutation that restores the division.
110+
111+
## 8. Downstream
112+
113+
Neither `cna` nor `mobile-eggbert` references `AggregateException` — zero sites in both. The
114+
rebuild requirement is recorded here for any future consumer.

modules/core/include/System/AggregateException.hpp

Lines changed: 95 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#pragma once
55
#include <functional>
6+
#include <typeinfo>
67
#include <initializer_list>
78
#include <memory>
89
#include <stdexcept>
@@ -23,6 +24,31 @@ namespace System {
2324
class AggregateException : public Exception {
2425
std::vector<std::exception_ptr> innerExceptions_;
2526

27+
/**
28+
* @brief The message the CALLER supplied, before composition.
29+
*
30+
* Ticket #2309 (SR-AUD-098 clauses C2/C3/C4), landed under `docs/StandingApprovals.md` SA-3.
31+
*
32+
* .NET stores the raw message and composes `Message` on every access
33+
* (`AggregateException.cs:339-360`); this port stored only the composed result. The ticket
34+
* measured why one string cannot serve both: closing C2 alone works, closing C3/C4 alone
35+
* works, and closing BOTH under one stored string yields
36+
* `"custom outer (a) (b) (a) (b)"` and **grows without bound under repeated `Flatten()`**.
37+
* A second field is the only escape, and it is exactly SA-3's case.
38+
*
39+
* The composition happens in the constructor rather than in the getter, and that is
40+
* observationally identical rather than a shortcut: `innerExceptions_` is fixed at
41+
* construction and never mutated, in this port and in .NET (`_innerExceptions` is a
42+
* `ReadOnlyCollection` assigned once). Composing on access would additionally require
43+
* `getMessageProperty()` to stop returning `const std::string&`, which is a public signature
44+
* change this needs no part of.
45+
*
46+
* `sizeof(AggregateException)` grows **192 → 224** and
47+
* `sizeof(UnobservedTaskExceptionEventArgs)` — a public header holding one by value —
48+
* **208 → 240**. Both are pinned by a layout test; consumers must rebuild.
49+
*/
50+
std::string rawMessage_;
51+
2652
/**
2753
* @brief Rejects a null entry in a collection of inner exceptions.
2854
*
@@ -98,40 +124,66 @@ class AggregateException : public Exception {
98124
// initializer is sequenced before every member initializer, so validating here
99125
// also protects innerExceptions_ in the constructors that build their message
100126
// from the same vector.
127+
return composeMessage("One or more errors occurred.", exs);
128+
}
129+
130+
/**
131+
* @brief .NET's `Message` composition, transcribed.
132+
*
133+
* @code
134+
* if (_innerExceptions.Length == 0) return base.Message;
135+
* sb.Append(base.Message); sb.Append(' ');
136+
* for (…) { sb.Append('('); sb.Append(_innerExceptions[i].Message); sb.Append(") "); }
137+
* sb.Length--; // AggregateException.cs:339-360
138+
* @endcode
139+
*
140+
* Ticket #2309 corrected two things here. First, the composition is **unconditional** when
141+
* there are inner exceptions: this port composed only when the message was the default one,
142+
* so `AggregateException("custom outer", {a, b})` reported `"custom outer"` and **discarded
143+
* the leaves**, which is C2. Second, the separator is `") ("` in both, but the trailing
144+
* `sb.Length--` means .NET ends at `")"` with **no** trailing space — reproduced by
145+
* appending `") "` and dropping the last character, rather than by special-casing the last
146+
* element, so the two spellings cannot drift.
147+
*/
148+
static std::string composeMessage(const std::string& raw,
149+
const std::vector<std::exception_ptr>& exs) {
150+
// Ahead of the loop below, because that loop is the first of the three
151+
// std::rethrow_exception call sites a null entry would reach. A base-class
152+
// initializer is sequenced before every member initializer, so validating here
153+
// also protects innerExceptions_ in the constructors that build their message
154+
// from the same vector.
101155
requireNoNullElements(exs);
102-
if (exs.empty()) return "One or more errors occurred.";
103-
std::string m = "One or more errors occurred. (";
104-
bool first = true;
156+
if (exs.empty()) return raw;
157+
std::string m = raw;
158+
m += ' ';
105159
for (auto& ep : exs) {
160+
m += '(';
106161
try { std::rethrow_exception(ep); }
107-
catch (const std::exception& e) {
108-
if (!first) m += ") (";
109-
m += e.what();
110-
first = false;
111-
} catch (...) {
112-
if (!first) m += ") (";
113-
m += "unknown error";
114-
first = false;
115-
}
162+
catch (const std::exception& e) { m += e.what(); }
163+
catch (...) { m += "unknown error"; }
164+
m += ") ";
116165
}
117-
m += ")";
166+
m.pop_back();
118167
return m;
119168
}
120169

121170
public:
122171
/** @brief Initializes a new instance with the default aggregate error message. */
123-
AggregateException() : Exception("One or more errors occurred.") {}
172+
AggregateException() : Exception("One or more errors occurred."),
173+
rawMessage_("One or more errors occurred.") {}
124174

125175
/** @brief Initializes a new instance with the specified error message. */
126-
explicit AggregateException(const std::string& message) : Exception(message) {}
176+
explicit AggregateException(const std::string& message)
177+
: Exception(message), rawMessage_(message) {}
127178

128179
/**
129180
* @brief Initializes a new instance with a collection of inner exceptions.
130181
* @throws System::ArgumentException if any entry is a null `std::exception_ptr`.
131182
*/
132183
explicit AggregateException(std::vector<std::exception_ptr> innerExceptions)
133184
: Exception(buildMessage(innerExceptions), firstValidatedInnerOf(innerExceptions)),
134-
innerExceptions_(std::move(innerExceptions)) {}
185+
innerExceptions_(std::move(innerExceptions)),
186+
rawMessage_("One or more errors occurred.") {}
135187

136188
/**
137189
* @brief Initializes a new instance with an initializer list of inner exceptions.
@@ -145,8 +197,10 @@ class AggregateException : public Exception {
145197
* @throws System::ArgumentException if any entry is a null `std::exception_ptr`.
146198
*/
147199
AggregateException(const std::string& message, std::vector<std::exception_ptr> innerExceptions)
148-
: Exception(message, firstValidatedInnerOf(innerExceptions)),
149-
innerExceptions_(std::move(innerExceptions)) {}
200+
: Exception(composeMessage(message, innerExceptions),
201+
firstValidatedInnerOf(innerExceptions)),
202+
innerExceptions_(std::move(innerExceptions)),
203+
rawMessage_(message) {}
150204

151205
/**
152206
* @brief Initializes a new instance with a message and a single inner exception.
@@ -159,8 +213,9 @@ class AggregateException : public Exception {
159213
* @throws System::ArgumentNullException if @p innerException is null.
160214
*/
161215
AggregateException(const std::string& message, std::exception_ptr innerException)
162-
: Exception(message, requireNonNullInner(innerException)),
163-
innerExceptions_({innerException}) {}
216+
: Exception(composeMessage(message, {innerException}), requireNonNullInner(innerException)),
217+
innerExceptions_({innerException}),
218+
rawMessage_(message) {}
164219

165220
/**
166221
* @brief Gets the read-only collection of inner exceptions that caused this aggregate exception.
@@ -263,7 +318,20 @@ class AggregateException : public Exception {
263318
}
264319
}
265320
}
266-
return AggregateException(std::move(flat));
321+
// #2309. .NET is
322+
// new AggregateException(GetType() == typeof(AggregateException) ? base.Message
323+
// : Message, ...)
324+
// -- AggregateException.cs:335
325+
// The RAW message for a plain AggregateException, the COMPOSED one for a derived type.
326+
// That discrimination is what stops repeated Flatten() from accreting leaf text: passing
327+
// the composed message here yields "custom outer (a) (b) (a) (b)" and grows without
328+
// bound, which is exactly the pathology this ticket measured before the raw field existed.
329+
//
330+
// typeid is the C++ counterpart of GetType(), and this class is polymorphic, so it reads
331+
// the DYNAMIC type -- a derived aggregate takes the other arm, as .NET's does.
332+
return AggregateException(
333+
typeid(*this) == typeid(AggregateException) ? rawMessage_ : getMessageProperty(),
334+
std::move(flat));
267335
}
268336

269337
/**
@@ -289,7 +357,12 @@ class AggregateException : public Exception {
289357
for (auto& ep : innerExceptions_) {
290358
if (!predicate(ep)) unhandled.push_back(ep);
291359
}
292-
if (!unhandled.empty()) throw AggregateException(std::move(unhandled));
360+
// #2309. Handle passes the COMPOSED message, not the raw one --
361+
// `throw new AggregateException(Message, unhandledExceptions.ToArray(), ...)`
362+
// (AggregateException.cs:281). It differs from Flatten deliberately and the two are
363+
// transcribed separately rather than being given one shared helper that would hide it.
364+
if (!unhandled.empty())
365+
throw AggregateException(getMessageProperty(), std::move(unhandled));
293366
}
294367
};
295368

modules/core/tests/System/Diagnostics/StopwatchTests.cpp

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -241,15 +241,29 @@ TEST(StopwatchDefinedArithmeticTests, Fix2326_FrequencyIsTheClocksOwnNotTheTimeS
241241
TEST(StopwatchDefinedArithmeticTests, Fix2326_TheResolutionThatWasLost) {
242242
// WHY THE OLD VALUE WAS A DEFECT AND NOT MERELY A DIFFERENT CHOICE. GetTimestamp() used to
243243
// divide the clock's nanosecond count by 100, so anything finer than 100 ns was truncated
244-
// away. Two timestamps taken back to back must now be able to differ by less than 100 units.
245-
long long minimumDelta = kLongMax;
246-
for (int i = 0; i < 200; ++i) {
247-
const long long a = Stopwatch::GetTimestamp();
248-
const long long b = Stopwatch::GetTimestamp();
249-
if (b > a) minimumDelta = std::min(minimumDelta, b - a);
250-
}
251-
EXPECT_LT(minimumDelta, 100LL)
244+
// away.
245+
//
246+
// REWRITTEN BY #2309's GATE RUN (2026-08-18). The first version took 200 pairs of back-to-back
247+
// timestamps and required the smallest positive delta to be under 100 units. That measures the
248+
// MACHINE, not the code: it passed eight times out of eight in isolation and failed once
249+
// inside a full gate, where every one of the 200 pairs can be preempted. A test that is
250+
// intermittently green is not evidence (#2352).
251+
//
252+
// The property is a UNIT, so it is asserted against a unit. GetTimestamp() samples
253+
// steady_clock's own epoch, so a division by 100 would put its value two orders of magnitude
254+
// below the same clock read directly -- a comparison of VALUES, immune to scheduling.
255+
const auto chronoNanoseconds = std::chrono::duration_cast<std::chrono::nanoseconds>(
256+
std::chrono::steady_clock::now().time_since_epoch())
257+
.count();
258+
const long long timestamp = Stopwatch::GetTimestamp();
259+
ASSERT_GT(chronoNanoseconds, 0LL) << "the clock's epoch is too close to now to discriminate";
260+
EXPECT_GT(timestamp, chronoNanoseconds / 2)
252261
<< "GetTimestamp() is still quantised to 100 ns -- the #2326 division is back";
262+
EXPECT_LT(timestamp / 2, chronoNanoseconds);
263+
264+
// The same statement from the other side: the declared frequency IS the clock's own, so one
265+
// unit is one nanosecond rather than one hundred.
266+
EXPECT_EQ(Stopwatch::Frequency, 1000000000LL);
253267
}
254268

255269
TEST(StopwatchDefinedArithmeticTests, GetElapsedTime_MinToMax_WrapsToMinusOne) {

0 commit comments

Comments
 (0)