feat: add cancellation and progress notification conformance scenarios - #450
feat: add cancellation and progress notification conformance scenarios#450elang2 wants to merge 2 commits into
Conversation
Add two new server conformance test scenarios covering utilities that previously had no test coverage: Cancellation (notifications/cancelled): - Server stability after cancellation of unknown request IDs - Cancellation of in-progress requests without server degradation - Rapid burst cancellation tolerance Progress notifications (notifications/progress): - Non-decreasing progress values with consistent totals - Token matching between request _meta and notifications - No spurious notifications when progressToken is absent - Cessation of notifications after request completion Everything-server additions: - test_tool_slow: configurable sleep duration for cancellation testing - test_tool_fast: immediate response for health-check verification - Fixed test_tool_with_progress to not send notifications when no progressToken is provided (was using ?? 0 fallback) Both scenarios are registered in pendingClientScenariosList until the everything-server tools are merged upstream. Tested against everything-server (spec 2025-06-18): cancellation: 4/4 checks passed (24 wire messages validated) progress-notifications: 5/5 checks passed (29 wire messages validated) Closes modelcontextprotocol#433, closes modelcontextprotocol#434
|
Thanks for sharing the replacement PR. I reviewed current head
This is a request to narrow claims and strengthen the test oracle, not a protocol-change proposal. |
- Cancellation: use timing evidence to distinguish actual cancellation from server ignoring unknown ID; INFO when healthy but not cancelled early - Cancellation: soften burst check to WARNING (spec says MAY ignore) - Cancellation: align docblock and description with spec MAY/SHOULD language - Progress: change monotonicity check from < to <= (spec says MUST increase) - Progress: move total-decrease and exceeds-total to observations only (no normative backing in current spec text)
|
Thanks for the detailed review. Pushed a fix addressing all five points. For (1), the SDK doesn't expose request IDs so I kept the assumption but now use timing as the actual assertion. SUCCESS requires the request to terminate early, otherwise it reports INFO acknowledging the server MAY ignore. For (2), softened to match spec language. Burst stability is now WARNING (SHOULD-level), not FAILURE. For (3), fixed. Comparison is now For (4), total-decrease and exceeds-total moved to For (5), PR body updated to 3 cancellation / 4 progress. Will attach a pinned run once the everything-server tools land. |
|
All five points from @joy7758 addressed and pushed. Burst is WARNING not FAILURE, progress uses <=, total warnings are non-scoring, check counts updated. Maintainer review welcome when someone has time. |
Summary
Adds two new server conformance scenarios covering utilities that previously had no test coverage:
Cancellation (notifications/cancelled) — 3 checks
Progress notifications (notifications/progress) — 4 checks
Also adds
test_tool_slowandtest_tool_fastto the everything-server for cancellation testing, and fixestest_tool_with_progressto not send notifications when no progressToken is provided.Both scenarios are registered in
pendingClientScenariosListuntil the everything-server tools are merged upstream.Closes #433, closes #434