Repository navigation
Conversation
Share drain lifecycle and session waiting with the TypeBox router. Add protobuf behavior coverage and document both router surfaces. Assisted-by: Replit
Only the runtime admission refusal may carry the reserved code, so a handler that already ran is never resent. Remap a handler-returned SERVER_DRAINING to UNCAUGHT_ERROR and log a warning. Assisted-by: Replit
| request: Uint8Array, | ||
| limits: ResendRefusedCallsOptions | undefined, | ||
| signal?: AbortSignal, | ||
| ): Promise<Result<MessageShape<Method['output']>, ClientError>> { |
There was a problem hiding this comment.
7 positional params that get forwarded straight into startMethodCall, plus the !limits branch inside. should this branch at the call site like the typebox client (resend ? rpcWithResends : handleProc) so the loop only exists when resend is on?
There was a problem hiding this comment.
Done in d4c4c2c. The loop is now one shared withRefusedCallResends(call, transport, serverId, limits, signal) in router/drain.ts, used by both routers. The protobuf unary case builds a call closure and only enters the loop when resendRefusedCalls is set, matching the TypeBox call site. unaryWithResends, the !limits branch, and the TypeBox rpcWithResends copy are gone.
|
|
||
| const resWritable = new WritableImpl<HandlerResponse>({ | ||
| writeCb: (response) => { | ||
| if (!response.ok && response.payload.code === SERVER_DRAINING_CODE) { |
There was a problem hiding this comment.
feels like a type thing not a runtime thing. can the handler's error type exclude ProtocolErrorCode instead of remapping this one code? a handler returning CANCEL or UNEXPECTED_DISCONNECT still goes through untouched here
There was a problem hiding this comment.
Agreed, moved it to the type level in d4c4c2c and dropped the runtime remap. Handler results (typed and raw) now use HandlerError, which excludes SERVER_DRAINING; a test with @ts-expect-error fails npm run check if the type is widened.
I kept the other protocol codes allowed on purpose. Handlers return ctx.cancel(...) (CANCEL) and forward request-stream errors like UNEXPECTED_DISCONNECT; the existing protobuf tests do both. Those codes never trigger resend. SERVER_DRAINING is also out of ProtocolErrorCode now, so request-stream errors cannot carry it and a client cancel with that code is rejected as on main.
| test('drain refuses calls before middleware and handler run', async () => { | ||
| // Risk: a refused call that ran would be unsafe to resend. | ||
| const middlewareEffects: Array<string> = []; | ||
| const f = setup(undefined, 1000, [ |
There was a problem hiding this comment.
setup(undefined, 1000, [...]) 👀 options object?
There was a problem hiding this comment.
Switched to setup({ resendRefusedCalls, middlewares }) in d4c4c2c.
| const success = { ok: true, payload: { text: 'ok' } }; | ||
|
|
||
| test('drain refuses calls before middleware and handler run', async () => { | ||
| // Risk: a refused call that ran would be unsafe to resend. |
There was a problem hiding this comment.
nit: these // Risk: lines read like the PR description, drop? (same for the rest of the file)
Replace the runtime remap with a handler error type that excludes SERVER_DRAINING; only the runtime refusal can send it. Share one resend loop between both routers so it exists only when resend is on. Use an options object in the protobuf drain test setup and drop the Risk lines. Assisted-by: Replit
Why
The protobuf router lacks the drain and resend added in #426. A server must refuse new work before middleware or handlers run so the client can safely resend it.
What changed
server.drain({ budgetMs })to the protobuf router. Refuse new calls withSERVER_DRAINING, wait for admitted unary and client-streaming calls, then close the transport when they finish or the budget expires.resendRefusedCalls: { maxResends, newSessionWaitMs }for protobuf unary calls. Only a drain refusal can be resent, and each resend needs a connected session with a new ID.Versioning
No wire envelope change or version bump.
ClientErrorCodegainsSERVER_DRAINING, so exhaustive client error switches need a new case. Handler results now useHandlerError, which excludesSERVER_DRAINING. A handler that returns a value typed as the widerClientError, for example by forwarding a client call's error, must map it first. The protobuf router is experimental; maintainers can decide whether that type change needs the breaking-ts box.Tests
npm run test:singleexited 0.npm run checkexited 0: TypeScript, Prettier, and ESLint passed.npm run check) use public APIs, fake timers, the existing service descriptor, and the mock transport withProtoCodec. The existing ten TypeBox drain tests are unchanged and pass.HandlerErrorto every client code:npm run checkfails with an unused@ts-expect-erroron the handler that returnsSERVER_DRAINING.Appendix
Decision Log
SERVER_DRAININGdirectly. The protobuf router already has a separateProtocolErrorCodeunion and runtime set for River's built-in errors alongside the gRPC-style codes (protobuf/errors.ts:37-60). No new metadata key, error-detail message, or genericUNAVAILABLEretry rule is needed.router/drain.ts: two states, the existing promise-resolver helper, bounded timers, explicit waiter cleanup, and one resend option contract. Each router supplies its own pending-call predicate and transport close operation.withRefusedCallResends, inrouter/drain.ts. Each router passes a closure that dispatches one call, so the loop runs only when resend is on and the session waiter stays private.MAX_TIMER_DELAY_MS, require a safe-integer resend count, and let non-positive counts disable resend.SERVER_DRAINING. This is enforced in types, not at runtime:HandlerErrorexcludes it from every handler result, raw handlers included.ProtocolErrorCodedoes not include it, so request-stream errors cannot carry it, and a client cancel with that code is rejected as on main. Other protocol codes stay allowed because handlers forward request-stream errors such asUNEXPECTED_DISCONNECTand returnctx.cancel(...); those codes never trigger resend.server.close()separately to dispose service resources.Facts verified in code
protobuf/server.ts:272-289,458-462,796-811: admission refusal, stream cleanup, and registration before middleware.protobuf/server.ts:185-193,346-348: unary/client-streaming pending predicate and shared drain entry point.protobuf/client.ts:154-178androuter/client.ts:305-320: only unary/RPC dispatch enters the shared loop, and only when resend is configured.protobuf/errors.ts:41-62,93-99,169-180andprotobuf/types.ts:18-21,128-144: client-visible codes, the handler error type, and its use in typed and raw handlers.protobuf/errors.ts:41-60andprotobuf/index.ts: the existing protocol error boundary and the refusal-code export.router/drain.ts:18-30,32-63,65-101,103-150: option validation, the shared resend loop, fresh-session waiting with listener/timer cleanup, and the two-state drain controller.router/server.ts:1078-1086retains the RPC/upload-only predicate; the shared loop keeps the dispatch-then-read-session ordering from the TypeBox resend loop in Add graceful server drain and opt-in resend for refused RPCs #426.transport/promises.tssupplies the existing promise-resolver helper.Review dispositions
anywere added. Existing TypeBox tests and the full protobuf matrix pass.SERVER_DRAININGafter running, and the client would resend it. 28f8a6e fixed it with a runtime remap. Per code review, d4c4c2c replaces that remap withHandlerError, so the code is rejected at compile time.// Risk:comments. Pass A and Pass B were rerun on this diff: SHIP. The full suite andnpm run checkpass.~ written by ⠕ Replit