Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 232f7ab0b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (true) { | ||
| if (params.abortSignal?.aborted) return; | ||
| const res = await signalCheck(params.baseUrl, 1000); | ||
| if (res.ok) return; | ||
| lastError = res.error ?? (res.status ? `HTTP ${res.status}` : "unreachable"); | ||
| const now = Date.now(); | ||
| if (now >= nextLogAt) { | ||
| const elapsedMs = now - started; | ||
| params.runtime.error?.( | ||
| danger(`daemon not ready after ${elapsedMs}ms (${lastError ?? "unknown error"})`), | ||
| ); | ||
| nextLogAt = now + logIntervalMs; | ||
| } | ||
| await new Promise((r) => setTimeout(r, 150)); |
There was a problem hiding this comment.
The new waitForSignalDaemonReady loop retries indefinitely and no longer throws on a timeout. If the daemon never comes up (e.g., bad baseUrl, port blocked, or signal-cli missing so the process can’t start), this promise will never resolve or reject, so monitorSignalProvider never completes and the gateway’s startAccount path will keep the account marked as running without ever setting lastError or allowing a restart. That’s a regression from the previous bounded wait: failures now look like a healthy running state while the daemon is dead. Consider reintroducing a timeout/throw or a maximum wait with an error so the caller can mark the account as failed (or explicitly transition to a “starting” state).
Useful? React with 👍 / 👎.
|
Landed via squash on main: aaa310c. Bounded signal/imsg transport readiness waits to 30s with periodic logging; shared helper + tests; add Szpadel to README. Thanks! |
|
Landed on main via squash: aaa310c. Closing PR. Thanks! |
Fix Enter key not submitting login form
2 participants
This pull request enhances the monitoring and startup logic for the Signal daemon by improving how readiness is detected and logged, and by adding comprehensive tests for the retry and logging behavior. The main focus is on making the system more robust in waiting for the daemon to become ready, with better error reporting over time.
Improvements to Signal Daemon Readiness Logic:
waitForSignalDaemonReadyto replace the previous fixed timeout with a logging mechanism that periodically reports when the daemon is not ready, continuing to retry indefinitely until the daemon responds or the operation is aborted. This includes configurable initial delay and log intervals for error reporting.monitorSignalProviderto use the newlogAfterMsandlogIntervalMsparameters, ensuring that logs are produced after a delay and at regular intervals while waiting for the daemon.Testing Enhancements:
monitor.daemon-retry.test.tsto verify that the monitoring logic retries until the daemon is ready and logs errors as expected after the initial wait period. The test uses mocks and fake timers to simulate daemon readiness and error conditions.