forward worker logs to main thread - #2078
Conversation
🦋 Changeset detectedLatest commit: bbf1598 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally makes sense, though I think my comments are worth considering prior to merging.
| onWorkerLogLevelChanged((level) => { | ||
| this.worker?.postMessage({ kind: 'setLogLevel', data: { level } }); | ||
| }); |
There was a problem hiding this comment.
🔴 Discarded encrypted rooms remain retained
Each setup adds a global log-level listener that permanently captures the manager, worker, and room. Recreating encrypted rooms accumulates retained rooms and redundant worker messages.
Prompt for agents
E2EEManager.setup registers an onWorkerLogLevelChanged callback and ignores the returned unsubscribe function. Because logger.ts stores callbacks in a module-global Set, that callback strongly retains the E2EEManager, its Room, and its Worker after the room is discarded. Reusing the manager with another Room also installs duplicate callbacks for the same worker. Add explicit listener ownership and lifecycle cleanup: retain the unsubscribe function, avoid duplicate registration, and invoke it when the manager/room is disposed or replaced. If E2EEManager currently has no teardown lifecycle, introduce one and call it from Room cleanup while preserving log-level propagation for active workers.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
5037c68 to
bbf1598
Compare
| break; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Encryption failures bypass log sinks
When CryptorEvent.Error lacks a worker log, the failure only reaches event listeners. Invalid-key and unknown-state errors bypass configured log sinks.
Prompt for agents
Some FrameCryptor error paths call emitThrottledError without first invoking workerLogger, including invalid-key decryption failures and unknown encryption state. E2EEManager.onWorkerMessage no longer logs error messages, so these errors only produce EncryptionError events and never reach console output or setLogExtension. Avoid duplicate logging while preserving coverage by either logging every CryptorEvent.Error at its worker source or marking error messages that have already been logged and retaining a manager-side fallback.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export class ErrorRateLimiter { | ||
| private lastAt: Map<string, number> = new Map(); | ||
|
|
||
| private counts: Map<string, number> = new Map(); |
There was a problem hiding this comment.
question: Do you need to have some way to clear keys in this map before .reset() is called? Is it possible that this map could become increasingly large across a long single room connection where there are a ton of diverse FrameCryptor errors? Or maybe that's low enough probability where it's most likely going to be fine. If you did want to add something here, maybe some sort of fairly long TTL for each key could be worthwhile.
2 participants
this allows
setLogExtension(and thus external log sinks) to capture the e2ee worker logs.