Stop each teardown stream on the observed post-wipe verdict instead of a fixed timer, which cuts enough wasted load to raise the crossing count and in-flight concurrency. Assert every teardown was straddled, and that a terminated session is never advertised as retryable.
Unified · Split
| @@ -101,6 +101,14 @@ const jsonRpcError = (body: string): { readonly code: number; readonly message: · 100 unmodified lines | |||
| 101 | } | 101 | } |
| 102 | }; | 102 | }; |
| 103 | 103 | ||
| 104 | /** | ||
| 105 | * The verdict a request gets once the destroy alarm has wiped the session's | ||
| 106 | * storage — i.e. the teardown this scenario is timing itself against is over. | ||
| 107 | * Matched on the protocol answer, not on any server internal. | ||
| 108 | */ | ||
| 109 | const isWipedVerdict = (probe: Probe): boolean => | ||
| 110 | probe.status === 404 && jsonRpcError(probe.body)?.message === "Session not found"; | ||
| 111 | |||
| 104 | const describeProbe = (probe: Probe): string => | 112 | const describeProbe = (probe: Probe): string => |
| 105 | `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`; | 113 | `+${probe.atMs}ms ${probe.status} ${probe.body.slice(0, 200)}`; |
| 106 | 114 | ||
| @@ -112,23 +120,36 @@ scenario( · 5 unmodified lines | |||
| 112 | const mcp = yield* Mcp; | 120 | const mcp = yield* Mcp; |
| 113 | 121 | ||
| 114 | // The fatal instant is the isolate abort at the end of the teardown, and a | 122 | // The fatal instant is the isolate abort at the end of the teardown, and a |
| 115 | // session only crosses it once — so cross it several times. Before the fix | 123 | // session only crosses it once — so cross it many times. A single crossing |
| 116 | // roughly one teardown in four leaked an unhandled 500. | 124 | // leaks the unhandled 500 only when a `validateMcpSessionOwner` RPC is |
| 117 | const sessions = 6; | 125 | // inside the object at exactly that instant, so the scenario has to make |
| 126 | // that likely rather than hope for it: `sessions` crossings, each blanketed | ||
| 127 | // by `concurrency` requests. At 6 × 8 the pre-fix bug reproduced in only | ||
| 128 | // about half of runs; the numbers below are the measured point where it | ||
| 129 | // reproduces every run without pushing the shared auth path into 403s. | ||
| 130 | const sessions = 12; | ||
| 118 | // Requests are kept continuously in flight rather than polled on a timer: | 131 | // Requests are kept continuously in flight rather than polled on a timer: |
| 119 | // the point is to have work ALREADY inside the session object when the | 132 | // the point is to have work ALREADY inside the session object when the |
| 120 | // termination lands, not to sample the window from outside it. | 133 | // termination lands, not to sample the window from outside it. |
| 121 | const concurrency = 8; | 134 | const concurrency = 24; |
| 122 | // Covers the deferred destroy alarm, the storage wipe and the abort that | 135 | // Ceiling, not the plan: the stream normally stops as soon as the teardown |
| 123 | // follows, and then stops. Kept tight on purpose: the traffic only has to | 136 | // is OBSERVED to have completed (below). This only bounds a teardown whose |
| 124 | // straddle the teardown, and a longer stream just piles avoidable load onto | 137 | // alarm never lands, so a hung platform fails the scenario instead of |
| 125 | // the shared auth path for no extra coverage. | 138 | // hanging it. |
| 126 | const streamAfterDeleteMs = 1_600; | 139 | const streamAfterDeleteMs = 3_000; |
| 140 | // Once the storage wipe is visible the abort has already happened, so the | ||
| 141 | // window this scenario exists to cover is behind us. Keep going briefly — | ||
| 142 | // the abort and the wipe are not the same instant — then stop, because | ||
| 143 | // every further request is pure load on the shared auth path. | ||
| 144 | const graceAfterWipeMs = 150; | ||
| 127 | // Enough in-flight requests to be mid-teardown, without racing the DELETE | 145 | // Enough in-flight requests to be mid-teardown, without racing the DELETE |
| 128 | // itself before the session is fully established. | 146 | // itself before the session is fully established. |
| 129 | const warmupMs = 250; | 147 | const warmupMs = 250; |
| 130 | 148 | ||
| 131 | const probes: Probe[] = []; | 149 | const probes: Probe[] = []; |
| 150 | // Per teardown, so the scenario can PROVE each crossing was straddled | ||
| 151 | // rather than assume it. | ||
| 152 | const perSession: Probe[][] = []; | ||
| 132 | 153 | ||
| 133 | // One identity per teardown, all minted BEFORE any load starts. Two | 154 | // One identity per teardown, all minted BEFORE any load starts. Two |
| 134 | // reasons, both about keeping this scenario's traffic off the shared auth | 155 | // reasons, both about keeping this scenario's traffic off the shared auth |
| @@ -145,9 +166,16 @@ scenario( · 10 unmodified lines | |||
| 145 | for (const bearer of bearers) { | 166 | for (const bearer of bearers) { |
| 146 | const sessionId = yield* Effect.promise(() => openSession(target.mcpUrl, bearer)); | 167 | const sessionId = yield* Effect.promise(() => openSession(target.mcpUrl, bearer)); |
| 147 | 168 | ||
| 169 | const thisSession: Probe[] = []; | ||
| 170 | perSession.push(thisSession); | ||
| 171 | |||
| 148 | yield* Effect.promise(async () => { | 172 | yield* Effect.promise(async () => { |
| 149 | const startedAt = Date.now(); | 173 | const startedAt = Date.now(); |
| 150 | let stopAt = Number.POSITIVE_INFINITY; | 174 | let hardStopAt = Number.POSITIVE_INFINITY; |
| 175 | // Set the first time the wiped-storage verdict is seen, which is the | ||
| 176 | // observable end of the teardown — the stream stops on this, not a | ||
| 177 | // timer. | ||
| 178 | let stopAfterWipeAt = Number.POSITIVE_INFINITY; | ||
| 151 | 179 | ||
| 152 | const probeOnce = async (): Promise<void> => { | 180 | const probeOnce = async (): Promise<void> => { |
| 153 | const at = Date.now() - startedAt; | 181 | const at = Date.now() - startedAt; |
| @@ -156,19 +184,27 @@ scenario( · 2 unmodified lines | |||
| 156 | sessionId, | 184 | sessionId, |
| 157 | body: TOOLS_LIST_REQUEST, | 185 | body: TOOLS_LIST_REQUEST, |
| 158 | }); | 186 | }); |
| 159 | probes.push({ | 187 | const probe: Probe = { |
| 160 | atMs: at, | 188 | atMs: at, |
| 161 | status: response.status, | 189 | status: response.status, |
| 162 | body: await response.text(), | 190 | body: await response.text(), |
| 163 | retryAfter: response.headers.get("retry-after"), | 191 | retryAfter: response.headers.get("retry-after"), |
| 164 | }); | 192 | }; |
| 193 | probes.push(probe); | ||
| 194 | thisSession.push(probe); | ||
| 195 | // "Session not found" is the post-wipe verdict: the destroy alarm has | ||
| 196 | // run, storage is gone, and the object has been aborted. Anything | ||
| 197 | // after this point is a request against an already-dead id. | ||
| 198 | if (isWipedVerdict(probe) && stopAfterWipeAt === Number.POSITIVE_INFINITY) { | ||
| 199 | stopAfterWipeAt = Date.now() + graceAfterWipeMs; | ||
| 200 | } | ||
| 165 | }; | 201 | }; |
| 166 | 202 | ||
| 167 | // One worker replenishes its request the moment the previous one | 203 | // One worker replenishes its request the moment the previous one |
| 168 | // settles, so the session object is never idle and the DELETE has to | 204 | // settles, so the session object is never idle and the DELETE has to |
| 169 | // land on top of real traffic. | 205 | // land on top of real traffic. |
| 170 | const worker = async (): Promise<void> => { | 206 | const worker = async (): Promise<void> => { |
| 171 | while (Date.now() < stopAt) await probeOnce(); | 207 | while (Date.now() < hardStopAt && Date.now() < stopAfterWipeAt) await probeOnce(); |
| 172 | }; | 208 | }; |
| 173 | const workers = Array.from({ length: concurrency }, () => worker()); | 209 | const workers = Array.from({ length: concurrency }, () => worker()); |
| 174 | 210 | ||
| @@ -181,7 +217,7 @@ scenario( · 6 unmodified lines | |||
| 181 | await terminate.text(); | 217 | await terminate.text(); |
| 182 | expect(terminate.status, "the client can terminate its session").toBe(200); | 218 | expect(terminate.status, "the client can terminate its session").toBe(200); |
| 183 | 219 | ||
| 184 | stopAt = Date.now() + streamAfterDeleteMs; | 220 | hardStopAt = Date.now() + streamAfterDeleteMs; |
| 185 | await Promise.all(workers); | 221 | await Promise.all(workers); |
| 186 | }); | 222 | }); |
| 187 | } | 223 | } |
| @@ -220,22 +256,110 @@ scenario( · 32 unmodified lines | |||
| 220 | "every rejection is a JSON-RPC error envelope the client can parse", | 256 | "every rejection is a JSON-RPC error envelope the client can parse", |
| 221 | ).toEqual([]); | 257 | ).toEqual([]); |
| 222 | 258 | ||
| 223 | // Proves the traffic actually straddled the teardown rather than finishing | 259 | // Coverage precondition, checked per teardown rather than in aggregate: a |
| 224 | // before it: the dead id has to have been reported dead at least once. | 260 | // run in which some session's stream finished before its object was torn |
| 225 | const reconnectVerdicts = probes.filter((probe) => probe.status === 404); | 261 | // down never entered the window this scenario exists to cover, and its |
| 262 | // green is worth nothing. Requiring EVERY crossing to have been observed is | ||
| 263 | // what stops the assertions above from passing vacuously. | ||
| 264 | const notStraddled = perSession | ||
| 265 | .map((session, index) => ({ index, wiped: session.some(isWipedVerdict) })) | ||
| 266 | .filter((entry) => !entry.wiped) | ||
| 267 | .map((entry) => `teardown #${entry.index + 1}`); | ||
| 226 | expect( | 268 | expect( |
| 227 | reconnectVerdicts.length, | 269 | notStraddled, |
| 228 | "the stream reached the terminated session and was told to reconnect", | 270 | "every teardown was still under load when its session object was destroyed", |
| 229 | ).toBeGreaterThan(0); | 271 | ).toEqual([]); |
| 230 | 272 | ||
| 231 | // A 503 here means "the platform is mid-reset, come back" — only actionable | 273 | // The disposition half of the contract, on the one failure this scenario |
| 232 | // if the client is told how long to wait, so it must carry Retry-After. | 274 | // can actually produce. Every platform failure reachable here is the |
| 233 | const retryableWithoutBackoff = probes.filter( | 275 | // session's own `ctx.abort` after a DELETE the client itself sent, and that |
| 234 | (probe) => probe.status === 503 && probe.retryAfter === null, | 276 | // id is dead for good — so the answer has to be "reconnect", never |
| 235 | ); | 277 | // "restarting, retry". A retryable verdict for a deliberately terminated |
| 278 | // session is worse than the 500 this PR removed: the client is told to keep | ||
| 279 | // asking, and the loop never ends. | ||
| 280 | const deadIdCalledRetryable = probes.filter((probe) => probe.status === 503); | ||
| 236 | expect( | 281 | expect( |
| 237 | retryableWithoutBackoff.map(describeProbe), | 282 | deadIdCalledRetryable.map(describeProbe), |
| 283 | "a session the client terminated is never advertised as retryable", | ||
| 284 | ).toEqual([]); | ||
| 285 | |||
| 286 | // A 503 here means "the platform is mid-reset, come back". It is only | ||
| 287 | // actionable if the client is told how long to wait, so it must carry | ||
| 288 | // Retry-After AND a positive delay — a `retry-after: 0`, or a header the | ||
| 289 | // renderer dropped, is the same dead end as no answer at all. | ||
| 290 | const badBackoff = probes.filter((probe) => { | ||
| 291 | if (probe.status !== 503) return false; | ||
| 292 | const seconds = probe.retryAfter === null ? null : Number(probe.retryAfter); | ||
| 293 | return seconds === null || !Number.isFinite(seconds) || seconds <= 0; | ||
| 294 | }); | ||
| 295 | expect( | ||
| 296 | badBackoff.map(describeProbe), | ||
| 238 | "every retryable rejection tells the client how long to back off", | 297 | "every retryable rejection tells the client how long to back off", |
| 239 | ).toEqual([]); | 298 | ).toEqual([]); |
| 299 | |||
| 300 | // The other half of the classifier's contract, and the half a teardown | ||
| 301 | // cannot demonstrate: a DEFINITIVE refusal must stay definitive. If the | ||
| 302 | // platform-failure path ever widens far enough to swallow ordinary | ||
| 303 | // rejections, these are the two answers that would silently turn into "the | ||
| 304 | // session is restarting, retry" and send a client into a loop it can never | ||
| 305 | // exit. | ||
| 306 | const liveIdentity = yield* target.newIdentity(); | ||
| 307 | const liveBearer = yield* mcp.mintBearer(emailOf(liveIdentity)); | ||
| 308 | const liveSessionId = yield* Effect.promise(() => openSession(target.mcpUrl, liveBearer)); | ||
| 309 | |||
| 310 | const strangerIdentity = yield* target.newIdentity(); | ||
| 311 | const strangerBearer = yield* mcp.mintBearer(emailOf(strangerIdentity)); | ||
| 312 | |||
| 313 | yield* Effect.gen(function* () { | ||
| 314 | const hijack = yield* Effect.promise(() => | ||
| 315 | mcpPost(target.mcpUrl, { | ||
| 316 | bearer: strangerBearer, | ||
| 317 | sessionId: liveSessionId, | ||
| 318 | body: TOOLS_LIST_REQUEST, | ||
| 319 | }), | ||
| 320 | ); | ||
| 321 | const hijackBody = yield* Effect.promise(() => hijack.text()); | ||
| 322 | expect( | ||
| 323 | hijack.status, | ||
| 324 | "another account's session id is refused outright, not advertised as retryable", | ||
| 325 | ).toBe(403); | ||
| 326 | expect(hijack.headers.get("retry-after"), "a refusal carries no backoff advice").toBeNull(); | ||
| 327 | expect(jsonRpcError(hijackBody)?.code, "the refusal is the session-ownership error").toBe( | ||
| 328 | -32003, | ||
| 329 | ); | ||
| 330 | |||
| 331 | const unauthenticated = yield* Effect.promise(() => | ||
| 332 | fetch(target.mcpUrl, { | ||
| 333 | method: "POST", | ||
| 334 | headers: { accept: JSON_AND_SSE, "content-type": "application/json" }, | ||
| 335 | body: JSON.stringify(TOOLS_LIST_REQUEST), | ||
| 336 | }), | ||
| 337 | ); | ||
| 338 | yield* Effect.promise(() => unauthenticated.text()); | ||
| 339 | expect( | ||
| 340 | unauthenticated.status, | ||
| 341 | "a missing credential is refused outright, not advertised as retryable", | ||
| 342 | ).toBe(401); | ||
| 343 | expect( | ||
| 344 | unauthenticated.headers.get("retry-after"), | ||
| 345 | "a refusal carries no backoff advice", | ||
| 346 | ).toBeNull(); | ||
| 347 | }).pipe( | ||
| 348 | // Runs even when an assertion above fails, so the extra session never | ||
| 349 | // outlives the scenario. `tryPromise` so a refused cleanup is a failure | ||
| 350 | // `ignore` can actually swallow — cleanup must not mask the real verdict. | ||
| 351 | Effect.ensuring( | ||
| 352 | Effect.tryPromise(async () => { | ||
| 353 | const closed = await fetch(target.mcpUrl, { | ||
| 354 | method: "DELETE", | ||
| 355 | headers: { | ||
| 356 | authorization: `Bearer ${liveBearer}`, | ||
| 357 | "mcp-session-id": liveSessionId, | ||
| 358 | }, | ||
| 359 | }); | ||
| 360 | await closed.text(); | ||
| 361 | }).pipe(Effect.ignore), | ||
| 362 | ), | ||
| 363 | ); | ||
| 240 | }), | 364 | }), |
| 241 | ); | 365 | ); |