fix(db): detect WSL2 and adjust pgserve startup timeout - #1044
rafaelcalassara wants to merge 10 commits into
Conversation
chore: rolling promotion dev -> main
fix(ci): hardcoded v3 prefix in version and release workflows
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pg-listen-notify-migration-should-b Fix automagik-dev#1004: docs: PG LISTEN/NOTIFY migration should be documented in s...
chore: rolling promotion dev -> main
release: v4.260404 — Claude Agent SDK executor + Omni bridge
Add WSL2 detection via /proc/version check and increase pgserve startup timeout from 15s to 30s on WSL2 systems. Use exponential backoff polling with 2s initial bootstrap delay to accommodate slower I/O during database initialization. Non-WSL2 systems (Mac, Linux, Windows) unchanged. - New module: src/lib/wsl2-detect.ts with memoized isWSL2() detection - Modified: src/lib/db.ts to use conditional timeout and bootstrap delay - Fixes pgserve timeout failures on WSL2 without affecting other platforms
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: fc0de38538
ℹ️ 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".
| mkdirSync(DATA_DIR, { recursive: true }); | ||
|
|
||
| const isWsl = isWSL2(); | ||
| const timeoutMs = isWsl ? 30000 : 15000; |
There was a problem hiding this comment.
Increasing daemon-side startup timeout to 30s on WSL2 here creates a mismatch with the CLI path in _ensurePgserve() that still gives up after 16s (const deadline = Date.now() + 16000). On slower WSL2 setups, the daemon can now succeed between 16–30s, but the initiating CLI command will still throw a timeout error even though pgserve comes up shortly after, which makes normal command execution fail spuriously in the target environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses WSL2-specific pgserve startup timeouts by detecting WSL2 at runtime and adjusting pgserve startup waiting behavior to better tolerate slower initialization on WSL2, while also updating mailbox module header documentation.
Changes:
- Added a memoized WSL2 detection helper (
isWSL2()) based on/proc/version. - Updated
pgservestartup logic to use a longer timeout on WSL2 and exponential backoff polling with an initial bootstrap delay. - Expanded
mailbox.tsheader comment with a migration note describing the move from file-based polling to PostgreSQL LISTEN/NOTIFY.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/lib/wsl2-detect.ts | Introduces WSL detection helper used to gate WSL-specific timeout behavior. |
| src/lib/db.ts | Applies WSL-aware pgserve startup timeout and adjusts health-check polling strategy. |
| src/lib/mailbox.ts | Updates module-level documentation about mailbox storage/delivery migration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Detect if running on WSL2 by checking /proc/version for "microsoft" or "wsl". | ||
| * Result is memoized for performance. | ||
| * | ||
| * @returns true if running on WSL2, false otherwise | ||
| */ | ||
| export function isWSL2(): boolean { | ||
| // Return memoized result if already computed | ||
| if (memoized !== null) return memoized; | ||
|
|
||
| try { | ||
| const procVersion = readFileSync('/proc/version', 'utf-8'); | ||
| memoized = procVersion.toLowerCase().includes('microsoft') || procVersion.toLowerCase().includes('wsl'); | ||
| return memoized; |
There was a problem hiding this comment.
isWSL2() currently checks /proc/version for "microsoft" or "wsl", which identifies WSL in general but does not distinguish WSL2 vs WSL1. Since the function name/docs are WSL2-specific, this can return true on WSL1 as well. Consider switching to a WSL2-specific signal (e.g., /proc/sys/kernel/osrelease containing "WSL2") or renaming the API to reflect what it actually detects.
| const initialBootstrapMs = isWsl ? 2000 : 100; | ||
| await new Promise((r) => setTimeout(r, initialBootstrapMs)); | ||
|
|
There was a problem hiding this comment.
This introduces a 100ms bootstrap delay on non-WSL platforms (initialBootstrapMs = ... : 100), which is a behavior change (previously health checks started immediately). If the intent is “Non-WSL2 systems unchanged” per the PR description, consider using 0 (or leaving the old behavior) for the non-WSL branch.
| const initialBootstrapMs = isWsl ? 2000 : 100; | ||
| await new Promise((r) => setTimeout(r, initialBootstrapMs)); | ||
|
|
||
| const deadline = Date.now() + timeoutMs; | ||
| let retryDelayMs = 100; |
There was a problem hiding this comment.
deadline is computed after the initial bootstrap sleep, so the total wait from spawn → timeout becomes initialBootstrapMs + timeoutMs, but the thrown error reports only timeoutMs seconds. Consider starting the deadline before the bootstrap delay, or include the bootstrap delay in the reported timeout to keep the error message accurate.
| const isWsl = isWSL2(); | ||
| const timeoutMs = isWsl ? 30000 : 15000; | ||
| const timeoutSec = timeoutMs / 1000; | ||
|
|
There was a problem hiding this comment.
WSL2 gets a longer pgserve startup timeout here, but the CLI path (_ensurePgserve step 4b) still times out waiting for the lockfile after 16s. If pgserve initialization ever takes >16s on WSL2, CLI commands can still fail even though the daemon continues starting successfully. Consider applying the same WSL2-aware timeout to the lockfile wait path as well.
| * - A 30-second fallback poll catches any notifications missed during | ||
| * reconnects or daemon restarts. |
There was a problem hiding this comment.
The header comment claims “A 30-second fallback poll catches any notifications missed…”, but this module doesn’t implement a fallback poll (subscribeDelivery only registers LISTEN/NOTIFY). Either remove that statement or move/clarify it to wherever the fallback polling is actually implemented so the docs match the code.
| * - A 30-second fallback poll catches any notifications missed during | |
| * reconnects or daemon restarts. |
| * | ||
| * The current implementation replaced file-based polling with PostgreSQL: | ||
| * - Messages are persisted to the `mailbox` table (durable, queryable). | ||
| * - A `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)` |
There was a problem hiding this comment.
Grammar: “A AFTER INSERT trigger” should be “An AFTER INSERT trigger”.
| * - A `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)` | |
| * - An `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)` |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces WSL2 environment detection to adjust pgserve startup timeouts, addressing slower I/O performance in that environment. It also includes documentation updates regarding the migration from file-based polling to PostgreSQL-based messaging in the mailbox module. The reviewer suggested a minor refactoring of the WSL2 detection logic to improve readability and efficiency.
| try { | ||
| const procVersion = readFileSync('/proc/version', 'utf-8'); | ||
| memoized = procVersion.toLowerCase().includes('microsoft') || procVersion.toLowerCase().includes('wsl'); | ||
| return memoized; | ||
| } catch { | ||
| // /proc/version not readable (non-Linux or permission issue) — assume not WSL2 | ||
| memoized = false; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
For improved readability and slightly better performance, you can refactor this block. Calling toLowerCase() once is more efficient, and the catch block can be made more concise.
try {
const procVersion = readFileSync('/proc/version', 'utf-8').toLowerCase();
memoized = procVersion.includes('microsoft') || procVersion.includes('wsl');
return memoized;
} catch {
// /proc/version not readable (non-Linux or permission issue) — assume not WSL2
return (memoized = false);
}
Closing — superseded by PR #1050Issue #1043 was fixed in PR #1050 (merged), which:
This is simpler and more flexible than WSL2 auto-detection — no new module needed, works for any slow-I/O environment. Why this PR can't be rebased/mergedBeyond being superseded, this PR's Thanks @rafaelcalassara for the investigation — the WSL2 detection approach was solid, the simpler fix just landed first. |
Add WSL2 detection via /proc/version check and increase pgserve startup timeout from 15s to 30s on WSL2 systems. Use exponential backoff polling with 2s initial bootstrap delay to accommodate slower I/O during database initialization. Non-WSL2 systems (Mac, Linux, Windows) unchanged.
Changes:
Testing:
Fixes #1043