-
Notifications
You must be signed in to change notification settings - Fork 0
fix: bound SQLite pool waits so Windows integration does not timeout #745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1217f51
62da7f8
88ec495
0aa5357
c2a6856
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| # Issue #743 — Windows `database_transaction_test.wfl` TIMEOUT | ||
|
|
||
| ## Risk class | ||
|
|
||
| R3 — lifecycle, timeouts, database locks, process shutdown. | ||
|
|
||
| ## Cause | ||
|
|
||
| The suite-versus-isolated discrepancy is a 30-second SQLite pool wait that | ||
| coincides with `scripts/run_integration_tests.ps1`'s unchanged program | ||
| deadline: | ||
|
|
||
| - sqlx default `acquire_timeout` is 30 seconds. | ||
| - `Pool::close` waits indefinitely for outstanding connections. | ||
| - File-backed pools used DELETE journal mode with five connections. | ||
| - The runner discarded child stdout/stderr, so a completed test report or a | ||
| database error could not be distinguished from a hang. | ||
|
|
||
| Isolated runs finish because they do not contend with a prior suite's file | ||
| locks, antivirus scan backlog, or leftover `tx.db` sidecars. The full Windows | ||
| suite does. | ||
|
|
||
| ## Red | ||
|
|
||
| Commit `1217f513` (`test: reproduce SQLite pool waits matching the 30s runner`). | ||
|
|
||
| ```text | ||
| cargo test --test database_transaction_test runner_deadline -- --nocapture --test-threads=1 | ||
| ``` | ||
|
|
||
| Both new tests failed for the intended reason after ~8 seconds: | ||
|
|
||
| - `file_backed_pool_does_not_wait_the_runner_deadline_when_busy` — sixth acquire | ||
| waited longer than 8s. | ||
| - `closing_a_file_backed_pool_does_not_wait_the_runner_deadline` — close with | ||
| an outstanding connection waited longer than 8s. | ||
|
|
||
| ## Green | ||
|
|
||
| After bounding acquire/close, enabling WAL, closing leftover CLI pools, and | ||
| exiting 0 after a successful `--test` run: | ||
|
|
||
| ```text | ||
| cargo test --test database_transaction_test -- --test-threads=1 | ||
| # 23 passed (including the two previously failing runner-deadline tests) | ||
|
|
||
| cargo test --test database_transaction_cli_test -- --nocapture | ||
| # 1 passed in 0.04s | ||
|
|
||
| cargo test --test database_test -- --test-threads=1 | ||
| # 20 passed | ||
|
|
||
| target/debug/wfl --test TestPrograms/database_transaction_test.wfl | ||
| # Total: 8 Passed: 8 exit 0 in 0.073s | ||
| ``` | ||
|
|
||
| Clippy (`--test database_transaction_test --test database_transaction_cli_test --bin wfl -- -D warnings`) and `python3 scripts/check_repo_hygiene.py --mode static` passed. The runner deadline remains 30 seconds. | ||
|
|
||
| ## Residual risk | ||
|
|
||
| Windows Defender or a leftover WAL from a killed process can still slow the | ||
| first open of a file-backed database. The five-second bound turns that into a | ||
| reported error plus retained child logs instead of a runner TIMEOUT. REPL | ||
| database handles are intentionally left open across commands. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # 2026-09-21 — File-backed SQLite waits vs the Windows integration runner (#743) | ||
|
|
||
| The canonical Windows runner timed out `TestPrograms/database_transaction_test.wfl` | ||
| after its unchanged 30-second deadline. Isolated runs of the same program | ||
| passed all eight assertions. The runner then deleted the child's stdout and | ||
| stderr, so a 30-second pool wait looked exactly like a hung process. | ||
|
|
||
| sqlx's default `acquire_timeout` is 30 seconds and `Pool::close` waits until | ||
| every connection is returned. A full file-backed pool, or a close with an | ||
| outstanding connection, therefore matches the runner deadline. File-backed | ||
| SQLite also used DELETE journal mode with a five-connection pool, which on | ||
| Windows stacks reserved-lock waits. | ||
|
|
||
| The failing regressions hold all five connections and then acquire or close; | ||
| both waited past 8 seconds before the runtime change. File-backed pools now | ||
| use WAL, a five-second busy/acquire wait, and a bounded close. The interpreter | ||
| closes leftover pools at shutdown, and a successful `--test` run exits the | ||
| process the same way a failing one already did. The runner keeps child logs | ||
| under `target/test-artifacts/integration-runner/` on timeout or failure. | ||
|
|
||
| Transaction commit and rollback rules are unchanged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,9 +230,9 @@ if (-not (Test-Path "TestPrograms")) { | |
| # Run with timeout to prevent hangs. Start-Process requires DISTINCT | ||
| # file targets for stdout and stderr — PowerShell 7 errors when the same | ||
| # path is reused for both, and "NUL" is not a valid redirect target | ||
| # there — so redirect to two temp files and discard them. (Redirecting | ||
| # both to a single "NUL" left the whole Windows integration command | ||
| # unrunnable, so its assertions never actually ran.) | ||
| # there. Keep copies under target/test-artifacts on timeout/failure | ||
| # so a 30-second SQLite wait is not mistaken for a silent hang | ||
| # (issue #743). | ||
| $outFile = New-TemporaryFile | ||
| $errFile = New-TemporaryFile | ||
| $process = Start-Process -FilePath ".\$BinaryPath" -ArgumentList $wflArgs -NoNewWindow -PassThru -RedirectStandardOutput $outFile.FullName -RedirectStandardError $errFile.FullName | ||
|
|
@@ -245,23 +245,45 @@ if (-not (Test-Path "TestPrograms")) { | |
| $isExpectedFail = ($ExpectedFailTests -contains $wflFile.Name) -or | ||
| ($wflFile.FullName -match '[\\/]TestPrograms[\\/]error_examples[\\/]') | ||
|
|
||
| $keepLogs = $false | ||
| if (-not $completed) { | ||
| # Test timed out | ||
| $process.Kill() | ||
| Write-Host "[ERROR] TIMEOUT $($wflFile.Name) (exceeded ${TestTimeout}s)" -ForegroundColor Red | ||
| $failedPrograms++ | ||
| $keepLogs = $true | ||
| } elseif ($isExpectedFail) { | ||
| if ($process.ExitCode -ne 0) { | ||
| Write-Host "[SUCCESS] PASS $($wflFile.Name) (expected failure, exit code: $($process.ExitCode))" -ForegroundColor Green | ||
| } else { | ||
| Write-Host "[ERROR] FAIL $($wflFile.Name) (expected a nonzero exit, got 0)" -ForegroundColor Red | ||
| $failedPrograms++ | ||
| $keepLogs = $true | ||
| } | ||
| } elseif ($process.ExitCode -eq 0) { | ||
| Write-Host "[SUCCESS] PASS $($wflFile.Name)" -ForegroundColor Green | ||
| } else { | ||
| Write-Host "[ERROR] FAIL $($wflFile.Name) (exit code: $($process.ExitCode))" -ForegroundColor Red | ||
| $failedPrograms++ | ||
| $keepLogs = $true | ||
| } | ||
|
|
||
| if ($keepLogs) { | ||
| $logDir = Join-Path "target\test-artifacts\integration-runner" $wflFile.Name | ||
| New-Item -ItemType Directory -Force -Path $logDir | Out-Null | ||
| Copy-Item $outFile.FullName (Join-Path $logDir "stdout.log") -ErrorAction SilentlyContinue | ||
| Copy-Item $errFile.FullName (Join-Path $logDir "stderr.log") -ErrorAction SilentlyContinue | ||
|
Comment on lines
+274
to
+275
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| Write-Host "[INFO] Retained child logs: $logDir" -ForegroundColor Yellow | ||
| $stdoutTail = Get-Content $outFile.FullName -Tail 40 -ErrorAction SilentlyContinue | ||
| $stderrTail = Get-Content $errFile.FullName -Tail 40 -ErrorAction SilentlyContinue | ||
| if ($stdoutTail) { | ||
| Write-Host "[INFO] stdout (last 40 lines):" -ForegroundColor Yellow | ||
| $stdoutTail | ForEach-Object { Write-Host $_ } | ||
| } | ||
| if ($stderrTail) { | ||
| Write-Host "[INFO] stderr (last 40 lines):" -ForegroundColor Yellow | ||
| $stderrTail | ForEach-Object { Write-Host $_ } | ||
| } | ||
| } | ||
| Remove-Item $outFile.FullName, $errFile.FullName -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,18 +19,30 @@ | |
| use super::value::Value; | ||
| use sqlx::mysql::{MySqlPoolOptions, MySqlRow}; | ||
| use sqlx::postgres::{PgPoolOptions, PgRow}; | ||
| use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions, SqliteRow}; | ||
| use sqlx::sqlite::{ | ||
| SqliteConnectOptions, SqliteJournalMode, SqlitePoolOptions, SqliteRow, SqliteSynchronous, | ||
| }; | ||
| use sqlx::{Column, Row, TypeInfo, ValueRef}; | ||
| use std::cell::RefCell; | ||
| use std::collections::HashMap; | ||
| use std::rc::Rc; | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
||
| mod schema; | ||
| pub use schema::SchemaTransaction; | ||
|
|
||
| const MAX_POOL_CONNECTIONS: u32 = 5; | ||
|
|
||
| /// Bound for SQLite acquire, busy, and close waits. | ||
| /// | ||
| /// sqlx defaults `acquire_timeout` to 30 seconds and `Pool::close` waits until | ||
| /// every connection is returned. Both match the integration runner's unchanged | ||
| /// 30-second program deadline, so a contended file-backed pool is reported as | ||
| /// `TIMEOUT database_transaction_test.wfl` instead of a database error | ||
| /// (issue #743). Schema transactions already use this same five-second bound. | ||
| pub const SQLITE_LOCK_WAIT: Duration = Duration::from_secs(5); | ||
|
|
||
| /// A connection pool to one of the supported database backends. | ||
| #[derive(Clone)] | ||
| pub enum DbPool { | ||
|
|
@@ -109,15 +121,23 @@ pub fn value_to_sql_param(value: &Value) -> Result<SqlParam, String> { | |
| pub async fn connect(url: &str) -> Result<DbPool, String> { | ||
| if url.starts_with("sqlite:") { | ||
| let options = if url == "sqlite::memory:" || url == "sqlite://:memory:" { | ||
| SqliteConnectOptions::new().in_memory(true) | ||
| SqliteConnectOptions::new() | ||
| .in_memory(true) | ||
| .busy_timeout(SQLITE_LOCK_WAIT) | ||
| } else { | ||
| let path = url | ||
| .strip_prefix("sqlite://") | ||
| .or_else(|| url.strip_prefix("sqlite:")) | ||
| .unwrap_or(url); | ||
| // WAL lets the five-connection pool share one file without stacking | ||
| // reserved-lock waits; DELETE journal mode on Windows is what turns | ||
| // those waits into a runner TIMEOUT (issue #743). | ||
| SqliteConnectOptions::new() | ||
| .filename(path) | ||
| .create_if_missing(true) | ||
| .journal_mode(SqliteJournalMode::Wal) | ||
| .synchronous(SqliteSynchronous::Normal) | ||
|
Comment on lines
+138
to
+139
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Read-only SQLite databases reject connections Opening a read-only SQLite file now requests Learn moreWAL mode is persistent SQLite database state. Selecting it during every file-backed connection requires write access to the database directory and can reject a database that was previously usable for reads. The earlier default journal mode allowed the same query-only connection without creating WAL sidecars. Example: A deployment mounts Recommended fix: Preserve a read-only-compatible path instead of unconditionally setting Was this helpful? React with 👍 or 👎 to provide feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For every file-backed SQLite database, forcing AGENTS.md reference: AGENTS.md:L3-L7 Useful? React with 👍 / 👎. |
||
| .busy_timeout(SQLITE_LOCK_WAIT) | ||
|
Comment on lines
135
to
+140
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: set -eu
printf '%s\n' '--- database.rs targeted sections ---'
sed -n '110,165p' src/interpreter/database.rs
printf '%s\n' '--- Cargo manifests and sqlx declarations ---'
rg -n -C 3 'sqlx|tokio' Cargo.toml Cargo.lock 2>/dev/null | head -120
printf '%s\n' '--- pool construction and SQLite connect call sites ---'
rg -n -C 4 'connect_with|SqlitePoolOptions|max_connections|journal_mode|busy_timeout' src/interpreter/database.rsRepository: WebFirstLanguage/wfl Length of output: 9809 🏁 Script executed: set -eu
printf '%s\n' '--- available sqlx source ---'
find "$HOME/.cargo/registry/src" -path '*sqlx-0.9.0/sqlite/src/*' -type f 2>/dev/null | head -20
printf '%s\n' '--- sqlx source references ---'
rg -n -C 5 'journal_mode|busy_timeout|connect_with|SqliteConnectOptions' "$HOME/.cargo/registry/src" -g '*sqlx-0.9.0*' 2>/dev/null | head -240Repository: WebFirstLanguage/wfl Length of output: 219 🌐 Web query:
💡 Result: <source_evidence> Citations:
🌐 Web query:
💡 Result: <source_evidence> Citations:
🏁 Script executed: set -eu
printf '%s\n' '--- database constants and connect callers ---'
sed -n '1,65p' src/interpreter/database.rs
rg -n -C 4 'database::connect|interpreter::database|connect\(' src tests 2>/dev/null | head -240
printf '%s\n' '--- local docs/cache references for PoolOptions ---'
rg -n -C 4 'struct PoolOptions|fn connect_with|min_connections|max_connections' "$HOME/.cargo/registry" 2>/dev/null | head -200Repository: WebFirstLanguage/wfl Length of output: 21109 🌐 Web query:
💡 Result: <source_evidence> Citations:
🏁 Script executed: set -eu
printf '%s\n' '--- production database connection callers ---'
rg -n -C 6 'database::connect|DbPool::|pub async fn connect|connect_database|open.*database|database.*open' src -g '*.rs'Repository: WebFirstLanguage/wfl Length of output: 26215 Retry WAL setup when concurrent opens target the same SQLite file. Serialize the first WAL transition, or retry it and re-read 🤖 Prompt for AI AgentsSource: Learnings |
||
| }; | ||
| // In-memory SQLite databases exist per connection, so the pool must | ||
| // not hand out more than one. | ||
|
|
@@ -128,6 +148,7 @@ pub async fn connect(url: &str) -> Result<DbPool, String> { | |
| }; | ||
| SqlitePoolOptions::new() | ||
| .max_connections(max_connections) | ||
| .acquire_timeout(SQLITE_LOCK_WAIT) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 In-memory SQLite waits fail early Every SQLite pool now receives Learn moreAn in-memory SQLite pool has one connection because each additional connection would hold a different database. A transaction or long query can legitimately occupy that connection while another concurrent handler waits. The new timeout changes those waits even though the documented and stated behavior only bounds file-backed pools. Example: Handler A holds an in-memory transaction for six seconds while handler B queries the same handle. Handler B previously waited for the connection; it now receives a pool timeout after five seconds. Recommended fix: Apply Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| .connect_with(options) | ||
| .await | ||
| .map(DbPool::Sqlite) | ||
|
|
@@ -414,11 +435,23 @@ pub async fn rollback(tx: DbTransaction) -> Result<(), String> { | |
| } | ||
|
|
||
| /// Close the pool, ending all connections. | ||
| /// | ||
| /// SQLite `close` is bounded: an outstanding connection must not keep the | ||
| /// process alive until the integration runner's 30-second deadline. | ||
| pub async fn close(pool: DbPool) { | ||
| match pool { | ||
| DbPool::Postgres(pool) => pool.close().await, | ||
| DbPool::MySql(pool) => pool.close().await, | ||
| DbPool::Sqlite(pool) => pool.close().await, | ||
| DbPool::Sqlite(pool) => { | ||
| if tokio::time::timeout(SQLITE_LOCK_WAIT, pool.close()) | ||
| .await | ||
| .is_err() | ||
| { | ||
| // Dropping the pool after the deadline lets the process finish | ||
| // instead of matching the runner TIMEOUT. Remaining workers are | ||
| // reaped when the process exits. | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: WebFirstLanguage/wfl
Length of output: 5914
🌐 Web query:
official .NET System.Diagnostics.Process Kill WaitForExit redirected output contract💡 Result:
<source_evidence>
Citations:
Wait for the killed process before retaining its logs.
When the timed wait expires,
$process.Kill()terminates theSystem.Diagnostics.Processasynchronously. The followingCopy-ItemandGet-Contentcalls can run before termination completes and miss output still being written to the redirected files. Call$process.WaitForExit()afterKill()and before copying or reading the files. This does not change the existing timeout measurement, although it cannot recover data lost fromKill()'s abnormal termination.🤖 Prompt for AI Agents