Skip to content

Commit b4b6f5d

Browse files
author
gophergogo
committed
Add unit tests for crash-fix contracts (#212)
Covers three invariants the preceding commits rely on: - Nested deferredDelete: a destructor that itself calls deferredDelete must not lose the inner object — it drains on a later pass. Guards the real-world pattern where a connection's destructor releases sub-objects that also need deferral. - body_timeout == 0 disables the body timer entirely. The client-mode HttpCodecFilter sets this so SSE response streams with long idle gaps between events don't trigger a false timeout. - Periodic timer re-arm on the same TimerPtr: the pattern McpServer's background tasks use. Confirms repeated fires and prompt cessation on disableTimer(), replacing the previous scheme that built a fresh timer inside each fire.
1 parent 75c9dc6 commit b4b6f5d

3 files changed

Lines changed: 116 additions & 0 deletions

File tree

tests/event/test_deferred_delete_ordering.cc

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,51 @@ TEST_F(DeferredDeleteOrderingTest, BatchDrainsAfterCallbackReturns) {
144144
for (auto* f : flags) delete f;
145145
}
146146

147+
// A Tracer variant that itself queues another Tracer for deferred deletion
148+
// when it runs. Models the real-world case where a connection's destructor
149+
// releases sub-objects that also need to defer (filters, session state).
150+
struct CascadingTracer : public DeferredDeletable {
151+
CascadingTracer(std::atomic<bool>& destroyed,
152+
Dispatcher& dispatcher,
153+
std::unique_ptr<DeferredDeletable> child)
154+
: destroyed_(destroyed),
155+
dispatcher_(dispatcher),
156+
child_(std::move(child)) {}
157+
~CascadingTracer() override {
158+
if (child_) {
159+
dispatcher_.deferredDelete(std::move(child_));
160+
}
161+
destroyed_ = true;
162+
}
163+
std::atomic<bool>& destroyed_;
164+
Dispatcher& dispatcher_;
165+
std::unique_ptr<DeferredDeletable> child_;
166+
};
167+
168+
// If a destructor re-enters deferredDelete, the dispatcher must drain the
169+
// newly-queued item on a subsequent pass instead of losing it.
170+
TEST_F(DeferredDeleteOrderingTest, NestedDeferredDeleteDuringDrainDrainsLater) {
171+
std::atomic<bool> outer_destroyed{false};
172+
std::atomic<bool> inner_destroyed{false};
173+
174+
auto inner = std::make_unique<Tracer>(inner_destroyed);
175+
auto outer = std::make_unique<CascadingTracer>(outer_destroyed, *dispatcher_,
176+
std::move(inner));
177+
178+
dispatcher_->post(
179+
[this, outer_raw = outer.release()]() mutable {
180+
dispatcher_->deferredDelete(
181+
std::unique_ptr<DeferredDeletable>(outer_raw));
182+
});
183+
184+
for (int i = 0; i < 400 && !inner_destroyed.load(); ++i) {
185+
std::this_thread::sleep_for(5ms);
186+
}
187+
EXPECT_TRUE(outer_destroyed.load()) << "outer must drain first";
188+
EXPECT_TRUE(inner_destroyed.load())
189+
<< "inner queued from outer's destructor must also drain";
190+
}
191+
147192
} // namespace
148193
} // namespace event
149194
} // namespace mcp

tests/event/test_timer_lifetime.cc

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,44 @@ TEST_F(TimerLifetimeTest, TimerCleanupOnDestruction) {
362362
EXPECT_FALSE(callback_executed);
363363
}
364364

365+
/**
366+
* Test: Periodic timer pattern — timer stored as a member, re-armed by its
367+
* own callback via enableTimer on the same TimerPtr. This is the pattern
368+
* McpServer's background tasks rely on; the previous code created a fresh
369+
* TimerPtr on each fire, which both leaked scheduling state and destroyed
370+
* the currently-firing timer from inside its own callback.
371+
*/
372+
TEST_F(TimerLifetimeTest, PeriodicReArmOnSameTimerFiresRepeatedly) {
373+
std::atomic<int> fire_count{0};
374+
event::TimerPtr timer;
375+
376+
auto interval = std::chrono::milliseconds(20);
377+
378+
// Capture the timer by reference so the callback re-arms the exact same
379+
// TimerPtr each tick — the pattern McpServer uses.
380+
timer = dispatcher_->createTimer([&timer, &fire_count, interval]() {
381+
fire_count++;
382+
timer->enableTimer(interval);
383+
});
384+
timer->enableTimer(interval);
385+
386+
runDispatcher();
387+
std::this_thread::sleep_for(std::chrono::milliseconds(120));
388+
389+
// Expect at least 4 fires in ~120ms at 20ms cadence. We're loose on the
390+
// upper bound because CI scheduling jitter can double this on loaded
391+
// runners.
392+
EXPECT_GE(fire_count.load(), 4)
393+
<< "member timer with self-rearm must keep firing";
394+
395+
// disableTimer must stop the cadence promptly.
396+
timer->disableTimer();
397+
int count_at_disable = fire_count.load();
398+
std::this_thread::sleep_for(std::chrono::milliseconds(80));
399+
EXPECT_LE(fire_count.load(), count_at_disable + 1)
400+
<< "disableTimer must prevent further fires";
401+
}
402+
365403
} // namespace
366404
} // namespace event
367405
} // namespace mcp

tests/filter/test_http_codec_state_machine.cc

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,39 @@ TEST_F(HttpCodecStateMachineTest, BodyTimeout) {
273273
expectState(HttpCodecState::Error);
274274
}
275275

276+
// A body_timeout of 0 must disable the body timer entirely. Client-mode
277+
// HTTP streams that carry SSE responses stay open indefinitely with
278+
// arbitrary gaps between server events; a non-zero body timeout would
279+
// fire on idle and tear the stream down. HttpCodecFilter's client-mode
280+
// constructor passes 0 for exactly this reason, and relies on the state
281+
// machine honoring "0 == disabled".
282+
TEST_F(HttpCodecStateMachineTest, BodyTimeoutDisabledWhenZero) {
283+
bool error_called = false;
284+
config_.body_timeout = 0ms;
285+
config_.error_callback = [&error_called](const std::string&) {
286+
error_called = true;
287+
};
288+
state_machine_ =
289+
std::make_unique<HttpCodecStateMachine>(*dispatcher_, config_);
290+
291+
state_machine_->handleEvent(HttpCodecEvent::RequestBegin);
292+
runFor(10ms);
293+
294+
state_machine_->setExpectRequestBody(true);
295+
state_machine_->handleEvent(HttpCodecEvent::RequestHeadersComplete);
296+
runFor(10ms);
297+
expectState(HttpCodecState::ReceivingRequestBody);
298+
299+
// Run well past what the default 200ms body timeout would be. With
300+
// body_timeout == 0 the timer must never have been armed, so we stay
301+
// in ReceivingRequestBody with no error.
302+
runFor(300ms);
303+
304+
EXPECT_FALSE(error_called)
305+
<< "body_timeout == 0 must not raise a timeout error";
306+
expectState(HttpCodecState::ReceivingRequestBody);
307+
}
308+
276309
TEST_F(HttpCodecStateMachineTest, IdleTimeout) {
277310
expectState(HttpCodecState::WaitingForRequest);
278311

0 commit comments

Comments
 (0)