Repository navigation
fix(client,talk): keep listen loops alive, real reqSeq for react - #239
Conversation
`Client.listen` starts one floating async IIFE per stream and `Polling`
starts the pusher loop the same way, so nothing observes their
rejections. A transport error, a decrypt failure or a throw from one of
the consumer's own listeners -- `emit` calls them synchronously and does
not guard them -- reached the runtime as "Uncaught (in promise)" and
took the whole host process down. A bot that listens is exactly the
process that cannot afford that.
Report the error on the log channel instead, the way push/conn.ts
already reports a failed connect: `log("LegyPusherError", { error })`.
Only the loop that failed stops; the other loop, and the process, keep
running, and the consumer sees the failure rather than a silent exit.
The report itself is guarded, for the same reason conn.ts guards its
own: `log` fans out to user-supplied listeners, so letting one throw
would put back the unhandled rejection the handler exists to prevent.
`initLegyPusher` already logs `LegyPusherError_cannot_init` before it
rethrows a failed first connect, so that path now reports twice -- the
connect that failed, then the loop that ended. Both are true and the
pair reads as the sequence it is.
(cherry picked from commit 66baa6e)
`sendMessage` decides whether to retry over E2EE by reading
`error.data?.code.toString()`. `InternalError.data` defaults to `{}` and
only the two request throw sites that see a `TalkException` fill in a
code, so for every other failure -- the client-closed guard, anything
carrying a bare `data` -- `code` is undefined and `.toString()` threw a
TypeError from inside the handler that was inspecting the error.
The caller then got "Cannot read properties of undefined (reading
'toString')" instead of the error that actually stopped the send, which
is the one thing a catch block must never do.
Read the code through `String(error.data?.code ?? "")` instead: absent
becomes "", non-string codes still stringify, and the retry decision is
unchanged for the E2EE codes it was written for.
(cherry picked from commit fdbed1a)
`react` hardcoded `reqSeq: 0`, so every reaction sent in a session carried the same sequence number. reqSeq is how the server tells one request apart from a retry of the earlier one, and every other talk request already draws it from `getReqseq()`, which keeps a monotonic counter per service in storage. Default the field to `await this.client.getReqseq()` -- the same "talk" bucket sendMessage and sendChatChecked use -- and accept an optional `reqSeq` for callers that drive the sequence themselves. Existing callers, including TalkMessage.react, are unaffected apart from now sending a distinct number per reaction. (cherry picked from commit b26a97c)
There was a problem hiding this comment.
🟡 Changes recommended
Polling.initLegyPusher() can leave islisten stuck true on initial connect failure, preventing any subsequent pusher restart and leaving streams permanently empty after a transient failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves runtime resilience of long-lived listening/push loops in @evex/linejs/linejs by preventing unhandled promise rejections from crashing the host process, and fixes two TalkService request correctness issues (E2EE retry code inspection and reaction request sequencing). It also adds targeted Deno tests to validate the new behavior.
Changes:
- Prevent unhandled rejections in
Client.listen()loops by catching and reporting failures viabase.log. - Prevent unhandled rejections from the Polling pusher startup by starting it through a guarded helper.
- Fix
TalkService.sendMessage()E2EE retry decision forInternalErrors without acode, and makeTalkService.react()use a real request sequence number (with optional override), with new tests.
File summaries
| File | Description |
|---|---|
| packages/linejs/client/client.ts | Adds guarded error reporting (.catch(...)) for floating listen loops to avoid process-killing unhandled rejections. |
| packages/linejs/client/listen.test.ts | Adds tests proving listen loops report failures (including throwing listeners) without unhandled rejections. |
| packages/linejs/base/service/talk/mod.ts | Fixes E2EE retry code inspection and makes react() use a monotonic reqSeq by default. |
| packages/linejs/base/service/talk/mod.test.ts | Adds tests covering codeless InternalError behavior, E2EE retry, and react() reqSeq semantics. |
| packages/linejs/base/polling/mod.ts | Starts the Legy pusher via a helper that catches/logs rejections instead of letting them go unobserved. |
| packages/linejs/base/polling/mod.test.ts | Adds tests ensuring pusher init failures/log-listener throws don’t cause unhandled rejections. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Reviewed the complete diff and adjacent polling/stream writer, event dispatch, E2EE retry, and sequence persistence paths. The original fixes are valid, but I reproduced five failing cases before applying follow-up commit 49ac26e:
Added 14 regression/boundary tests including failed persistence recovery, explicit reqSeq=0, lossless large message IDs, missing/null/numeric error codes, explicit E2EE retry limits, and a throwing logger. Validation: repository deno test -A = 344 passed / 0 failed; Node 24 transport tests = 2 passed; linejs and linejs-types JSR dry-runs passed; touched TypeScript formatted and git diff --check passed. Waiting for CI on 49ac26e before merge and v3.4.1 release. Scope: no automatic retry of terminal initial connection errors; synchronous handler errors skip the affected event. Async event/log handlers must handle their own promise rejections (documented). No live LINE account verification performed. |
|
Thanks for the follow-up commit, and for merging. All four cases are real. The stuck Verified the merged tree here: repository root |
…them unhandled `_OnPingCallback` fires `talk.noop()` every third ping (every 90 s) without a catch, and Conn calls `_OnPushResponse` without awaiting it, so a request that hits `config.timeout` rejects with an `AbortSignal.timeout` TimeoutError that nobody observes. Node and Deno treat that as an unhandled rejection and terminate the host process. Catch both and report them through `ConnManager.log` as `noop failed` / `fetchMyEvents failed`, keeping the connection alive. As in #232 and #239, the report is wrapped in try/catch because `log` fans out to user-supplied listeners and a throwing listener would otherwise bring the unhandled rejection back.
Summary
Three fixes for the process that listens: two failures that kill it outright, and a reaction request that always sends sequence number 0. They are independent of each other and of the earlier pull requests from this fork; happy to split them if you would rather review them separately.
1. A failing listen loop takes the host process down
Client.listenstarts one floating async IIFE per stream (packages/linejs/client/client.ts:136-177for talk,:180-196for square), andPollingstarts the pusher the same way (packages/linejs/base/polling/mod.ts:179,:185):Nothing observes those promises. A transport error from the event stream, a decrypt failure on a single message, or a throw from one of the consumer's own listeners —
emitcalls them synchronously and does not guard them — reaches the runtime asand terminates the process.
initLegyPusherhas the same problem from the other side: it rethrows when the firstinitializeConnfails (base/polling/mod.ts:160-164), and the two call sites do not await it, so that rejection has nowhere to go either. A bot whose whole job is to listen dies from one transient failure, and the code that asked for the stream never learns why.Fix. Give each loop a
.catchthat reports on the log channel, the waybase/push/conn.ts:137already reports a failed connect:log("LegyPusherError", { error }). Only the loop that failed stops; the other loop, and the process, keep running. InPollingthis lives in a small#startLegyPusher()so bothlistenTalkEventsandlistenSquareEventsget it.The report itself is wrapped in
try {} catch {}for the same reasonconn.tsguards its own:logfans out to user-supplied listeners, so an unguarded report would put back the unhandled rejection the handler exists to prevent.One consequence worth naming:
initLegyPusheralready logsLegyPusherError_cannot_initbefore rethrowing a failed first connect, so that path now reports twice — the connect that failed, then the loop that ended. Both are true and the pair reads as the sequence it is; the alternative was inventing a third log type that existing consumers cannot anticipate.2. The E2EE retry check throws inside its own catch
sendMessagedecides whether to retry over E2EE by reading the error's code (packages/linejs/base/service/talk/mod.ts:172-183):InternalError.datadefaults to{}(base/core/utils/error.ts:7) and only the two throw sites that carry aTalkExceptionfill in acode. For every otherInternalError— the client-closed guard atbase/request/mod.ts:83, for one —codeis undefined,.toString()throws from inside the handler that was inspecting the error, and the caller getsinstead of the error that actually stopped the send.
Fix. Read it as
String(error.data?.code ?? "").includes("E2EE"): absent becomes"", non-string codes still stringify, and the decision is unchanged for the E2EE codes it was written for.3.
reactalways sends reqSeq 0TalkService.reacthardcodesreqSeq: 0(packages/linejs/base/service/talk/mod.ts:847), so every reaction sent in a session carries the same sequence number, while every other talk request draws one fromgetReqseq()(base/core/mod.ts:234), the monotonic per-service counter kept in storage.Fix. Default the field to
await this.client.getReqseq()— the same"talk"bucketsendMessage(:142) and the compact senders (:216,:240) use — and accept an optionalreqSeqfor callers that drive the sequence themselves. The change is additive:TalkMessage.react(client/features/message/talk.ts:106) is untouched and now simply sends a distinct number per reaction.Testing
New
packages/linejs/client/listen.test.ts, 4 cases against a stub polling source:listen()reports oneLegyPusherErrorcarrying the original error, the event delivered before the failure still reached the consumer, and no unhandled rejection is raised. Onmainit is, and Deno's default test sanitizers report that as a failure the same way the runtime reports it as a crash.emit→ reported, process survives.loglistener that itself throws → still no unhandled rejection.packages/linejs/base/polling/mod.test.ts, 2 added cases:listenTalkEventswith a pusher that cannot connect reports["LegyPusherError_cannot_init", "LegyPusherError"]and still returns its stream;listenSquareEventssurvives a throwingloglistener.New
packages/linejs/base/service/talk/mod.test.ts, 5 cases:sendMessagerethrows a codelessInternalErrorunchanged (theTypeErrorabove onmain), still retries with E2EE when the code asks for it, still rethrows a non-E2EE code without retrying;reacttakes its sequence number from the talk bucket, and honours a caller-supplied one — both decoding the written thrift the waybase/service/relation/mod.test.tsdoes.cd packages/linejs && deno test -A— 286 passed, 0 failed (275 onmain@ 8bd2f77). Repository-rootdeno test -A— 330 passed, 0 failed.deno fmt --checkanddeno check packages/linejs/client/mod.tsclean.client/features/call/stun.test.tsneeds a live UDP round trip tostun.l.google.comand failed once here on a dropped packet; it passes on a rerun and is untouched by this change.