Skip to content

Commit 6e40296

Browse files
pcarletonpcarleton
committed
fix: check expected notification count, improve failure messages
- Tests now verify each client receives all 3 expected progress notifications (not just > 0) - Failure messages explain what happened: - 'Received 1/3 expected progress notifications (2 likely routed to another client's stream)' - 'Received 4 notification(s) from other clients: fuzz-client-0 (2x), fuzz-client-1 (2x) — the server wrote them to this client's SSE stream'
1 parent d334196 commit 6e40296

1 file changed

Lines changed: 120 additions & 92 deletions

File tree

‎src/scenarios/server/session-isolation.ts‎

Lines changed: 120 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -657,103 +657,104 @@ export class NotificationIsolationScenario implements ClientScenario {
657657

658658
// -- Analyze results --
659659

660-
// Extract progress tokens from notifications on each stream
661-
const progressTokensOnA = streamA.notifications
662-
.filter((n) => n.method === 'notifications/progress')
663-
.map((n) => n.params?.progressToken);
664-
665-
const progressTokensOnB = streamB.notifications
666-
.filter((n) => n.method === 'notifications/progress')
667-
.map((n) => n.params?.progressToken);
668-
669-
// A should only have 'client-a-progress' tokens
670-
const aHasOwnNotifications = progressTokensOnA.some(
671-
(t) => t === 'client-a-progress'
672-
);
673-
const aHasLeakedNotifications = progressTokensOnA.some(
674-
(t) => t === 'client-b-progress'
675-
);
660+
// The test_tool_with_progress spec requires 3 notifications (0, 50, 100)
661+
const EXPECTED_NOTIFICATION_COUNT = 3;
662+
663+
// Categorize notifications on each stream
664+
const analyzeStream = (
665+
stream: SSEStreamResult,
666+
ownToken: string,
667+
otherToken: string,
668+
label: string
669+
) => {
670+
const allProgress = stream.notifications.filter(
671+
(n) => n.method === 'notifications/progress'
672+
);
673+
const ownCount = allProgress.filter(
674+
(t) => t.params?.progressToken === ownToken
675+
).length;
676+
const foreignTokens = allProgress
677+
.filter((t) => t.params?.progressToken === otherToken)
678+
.map((t) => t.params?.progressToken);
679+
const gotResponse = stream.response != null;
676680

677-
// B should only have 'client-b-progress' tokens
678-
const bHasOwnNotifications = progressTokensOnB.some(
679-
(t) => t === 'client-b-progress'
680-
);
681-
const bHasLeakedNotifications = progressTokensOnB.some(
682-
(t) => t === 'client-a-progress'
683-
);
681+
const errors: string[] = [];
682+
if (foreignTokens.length > 0) {
683+
errors.push(
684+
`${label} received ${foreignTokens.length} notification(s) ` +
685+
`belonging to the other client — the server wrote them to ` +
686+
`the wrong SSE stream`
687+
);
688+
}
689+
if (!gotResponse) {
690+
errors.push(
691+
`${label} received no final response (response may have been ` +
692+
`sent to the other client's stream)`
693+
);
694+
}
695+
if (ownCount < EXPECTED_NOTIFICATION_COUNT) {
696+
const missing = EXPECTED_NOTIFICATION_COUNT - ownCount;
697+
errors.push(
698+
`${label} received ${ownCount}/${EXPECTED_NOTIFICATION_COUNT} ` +
699+
`expected progress notifications (${missing} likely routed ` +
700+
`to the other client's stream)`
701+
);
702+
}
684703

685-
const aGotResponse = streamA.response != null;
686-
const bGotResponse = streamB.response != null;
704+
return {
705+
ownCount,
706+
foreignCount: foreignTokens.length,
707+
gotResponse,
708+
ok:
709+
foreignTokens.length === 0 &&
710+
gotResponse &&
711+
ownCount >= EXPECTED_NOTIFICATION_COUNT,
712+
errors
713+
};
714+
};
687715

688-
const noLeaks = !aHasLeakedNotifications && !bHasLeakedNotifications;
689-
const bothGotResponses = aGotResponse && bGotResponse;
690-
const bothGotOwnNotifications =
691-
aHasOwnNotifications && bHasOwnNotifications;
716+
const a = analyzeStream(
717+
streamA,
718+
'client-a-progress',
719+
'client-b-progress',
720+
'Client A'
721+
);
722+
const b = analyzeStream(
723+
streamB,
724+
'client-b-progress',
725+
'client-a-progress',
726+
'Client B'
727+
);
692728

693-
const errors: string[] = [];
694-
if (aHasLeakedNotifications) {
695-
errors.push(
696-
`Client A received Client B's progress notifications (tokens: ${progressTokensOnA.join(', ')})`
697-
);
698-
}
699-
if (bHasLeakedNotifications) {
700-
errors.push(
701-
`Client B received Client A's progress notifications (tokens: ${progressTokensOnB.join(', ')})`
702-
);
703-
}
704-
if (!aGotResponse) {
705-
errors.push(
706-
'Client A did not receive a final response (possible cross-wiring)'
707-
);
708-
}
709-
if (!bGotResponse) {
710-
errors.push(
711-
'Client B did not receive a final response (possible cross-wiring)'
712-
);
713-
}
714-
if (!aHasOwnNotifications && aGotResponse) {
715-
errors.push(
716-
'Client A received no progress notifications (may have been routed to another client)'
717-
);
718-
}
719-
if (!bHasOwnNotifications && bGotResponse) {
720-
errors.push(
721-
'Client B received no progress notifications (may have been routed to another client)'
722-
);
723-
}
729+
const allErrors = [...a.errors, ...b.errors];
724730

725731
checks.push({
726732
id: 'notification-isolation',
727733
name: 'NotificationIsolation',
728734
description:
729735
'In-request progress notifications are routed to the correct client',
730-
status:
731-
noLeaks && bothGotResponses && bothGotOwnNotifications
732-
? 'SUCCESS'
733-
: 'FAILURE',
736+
status: a.ok && b.ok ? 'SUCCESS' : 'FAILURE',
734737
timestamp,
735738
specReferences: SPEC_REFERENCES,
736739
details: {
737740
clientA: {
738741
sessionId: sessionIdA || '(none)',
739742
progressToken: 'client-a-progress',
740-
notificationCount: progressTokensOnA.length,
741-
tokensReceived: progressTokensOnA,
742-
hasOwnNotifications: aHasOwnNotifications,
743-
hasLeakedNotifications: aHasLeakedNotifications,
744-
gotResponse: aGotResponse
743+
ownNotifications: a.ownCount,
744+
foreignNotifications: a.foreignCount,
745+
expectedNotifications: EXPECTED_NOTIFICATION_COUNT,
746+
gotResponse: a.gotResponse
745747
},
746748
clientB: {
747749
sessionId: sessionIdB || '(none)',
748750
progressToken: 'client-b-progress',
749-
notificationCount: progressTokensOnB.length,
750-
tokensReceived: progressTokensOnB,
751-
hasOwnNotifications: bHasOwnNotifications,
752-
hasLeakedNotifications: bHasLeakedNotifications,
753-
gotResponse: bGotResponse
751+
ownNotifications: b.ownCount,
752+
foreignNotifications: b.foreignCount,
753+
expectedNotifications: EXPECTED_NOTIFICATION_COUNT,
754+
gotResponse: b.gotResponse
754755
}
755756
},
756-
errorMessage: errors.length > 0 ? errors.join('; ') : undefined
757+
errorMessage: allErrors.length > 0 ? allErrors.join('; ') : undefined
757758
});
758759
} catch (error) {
759760
checks.push({
@@ -877,34 +878,60 @@ export class NotificationIsolationFuzzScenario implements ClientScenario {
877878

878879
// -- Analyze results: one check per client --
879880

881+
// The test_tool_with_progress spec requires 3 notifications
882+
const EXPECTED_NOTIFICATION_COUNT = 3;
883+
880884
for (const { index, stream } of results) {
881885
const expectedToken = `fuzz-client-${index}`;
882-
const progressTokens = stream.notifications
883-
.filter((n) => n.method === 'notifications/progress')
884-
.map((n) => n.params?.progressToken);
885-
886-
const ownTokens = progressTokens.filter((t) => t === expectedToken);
887-
const foreignTokens = progressTokens.filter(
888-
(t) => t !== expectedToken && t !== undefined
886+
const allProgress = stream.notifications.filter(
887+
(n) => n.method === 'notifications/progress'
889888
);
889+
const ownCount = allProgress.filter(
890+
(n) => n.params?.progressToken === expectedToken
891+
).length;
892+
893+
// Identify foreign tokens and count how many of each
894+
const foreignTokenCounts = new Map<string, number>();
895+
for (const n of allProgress) {
896+
const token = n.params?.progressToken;
897+
if (token !== expectedToken && token !== undefined) {
898+
foreignTokenCounts.set(
899+
token,
900+
(foreignTokenCounts.get(token) || 0) + 1
901+
);
902+
}
903+
}
890904

891905
const gotResponse = stream.response != null;
892-
const hasLeak = foreignTokens.length > 0;
893-
const hasOwnNotifications = ownTokens.length > 0;
894-
const ok = !hasLeak && gotResponse && hasOwnNotifications;
906+
const foreignCount = allProgress.length - ownCount;
907+
const ok =
908+
foreignCount === 0 &&
909+
gotResponse &&
910+
ownCount >= EXPECTED_NOTIFICATION_COUNT;
895911

896912
const errors: string[] = [];
897-
if (hasLeak) {
913+
914+
if (foreignCount > 0) {
915+
const leaked = Array.from(foreignTokenCounts.entries())
916+
.map(([token, count]) => `${token} (${count}x)`)
917+
.join(', ');
898918
errors.push(
899-
`Received foreign notifications: ${foreignTokens.join(', ')}`
919+
`Received ${foreignCount} notification(s) from other clients: ${leaked} ` +
920+
`— the server wrote them to this client's SSE stream`
900921
);
901922
}
902923
if (!gotResponse) {
903-
errors.push('No final response (possible cross-wiring)');
924+
errors.push(
925+
'No final response (response may have been sent to another ' +
926+
"client's stream)"
927+
);
904928
}
905-
if (!hasOwnNotifications && gotResponse) {
929+
if (ownCount < EXPECTED_NOTIFICATION_COUNT) {
930+
const missing = EXPECTED_NOTIFICATION_COUNT - ownCount;
906931
errors.push(
907-
'No progress notifications received (notifications may have been routed to another client)'
932+
`Received ${ownCount}/${EXPECTED_NOTIFICATION_COUNT} expected ` +
933+
`progress notifications (${missing} likely routed to another ` +
934+
"client's stream)"
908935
);
909936
}
910937

@@ -918,8 +945,9 @@ export class NotificationIsolationFuzzScenario implements ClientScenario {
918945
details: {
919946
clientIndex: index,
920947
expectedToken,
921-
ownNotifications: ownTokens.length,
922-
foreignNotifications: foreignTokens.length,
948+
ownNotifications: ownCount,
949+
expectedNotifications: EXPECTED_NOTIFICATION_COUNT,
950+
foreignNotifications: foreignCount,
923951
gotResponse
924952
},
925953
errorMessage: errors.length > 0 ? errors.join('; ') : undefined

0 commit comments

Comments
 (0)