diff --git a/linux/ROADMAP.md b/linux/ROADMAP.md index d99e09fc81..7f27a4239a 100644 --- a/linux/ROADMAP.md +++ b/linux/ROADMAP.md @@ -182,7 +182,7 @@ What has to hold before the first release, for a user on GNOME 50 with Ubuntu 26 | 2 | Connects to their everyday PostgreSQL, MySQL, MariaDB, SQL Server, ClickHouse or SQLite database | The supported releases and why others are out, in the README's [supported servers](README.md#supported-servers). CI runs every driver's Docker suite on each tested release, MySQL 8.0 and SQL Server 2017 and 2019 among them | Nothing | | 3 | Browses tables of real-world data (dates, decimals, JSON, UUIDs, NULLs) and sees correct values | Per-engine decoding with the type fallback and unsupported-type conformance cases ([ADR 0014](docs/decisions/0014-lossless-values-and-dialects.md)). A value corpus on every engine, MariaDB too: dates and times at their extremes and in other zones, decimals, JSON, UUIDs, Unicode and binary values, read back through a table tab, Export All and an editor run | Nothing | | 4 | Runs typical queries against tables of a million rows or more without running out of memory or freezing | The 10,000-row editor limit with Export All, and driver tests over 2,000,000 rows. Pages seek past the edge row's key, or walk PostgreSQL heap block windows for a table with no key; OFFSET only under a user sort, a restored deep offset or a partitioned keyless table. Docker tests page 60k and 25k-row tables both ways and check the plan. A 1,000,000-row table is paged to its end and exported on PostgreSQL, MySQL, MariaDB, SQL Server, ClickHouse and SQLite, with bounds on time and on resident peak growth. SQLite's case runs in the unit suite in a test binary of its own, so the peak is its alone | Nothing | -| 5 | Edits data in place, and the changes commit correctly even when the connection or the app misbehaves | Write, row guard and commit resolution conformance on every engine, with the fault fakes and the grid save Docker test. `kill_during_save` kills TablePro halfway through a SQLite save: the file keeps none of it, and the next launch restores the session with its history, settings and saved tabs | A save into a SQLite file another program holds locked fails as `DriverError::Busy`, whose message says the server has no room for another connection | +| 5 | Edits data in place, and the changes commit correctly even when the connection or the app misbehaves | Write, row guard and commit resolution conformance on every engine, with the fault fakes and the grid save Docker test. `kill_during_save` kills TablePro halfway through a SQLite save: the file keeps none of it, and the next launch restores the session with its history, settings and saved tabs. A save into a SQLite file another program holds locked fails as `Busy(FileLocked)`, which says another program or an editor tab holds the file's lock, and leaves the file as it was whether the lock shows at `BEGIN IMMEDIATE` or at the `COMMIT` | Nothing | | 6 | Trusts the app with credentials: TLS, SSH tunnels, no password in plain text | Verify full by default, the TLS conformance suite, the Secret Service, and statement logging silenced | An outside security review | | 7 | Recovers when the network drops or the database restarts | Health monitoring, backoff and replay rules with their tests, a stopped container in the liveness tests, and a session test that restarts PostgreSQL: the read in flight runs again, the save in flight does not and leaves no row | Nothing | | 8 | Works with a screen reader, the keyboard alone and larger text | Label audits of the picker, the connect pages, the workspace, the connect dialog and preferences, labelled cells and real-key tests. Sorting from the cell menu, named filter fields and cell editors, focus handed on when a control hides, and announced query results and errors. Columns resize from the keyboard | A column resized from the keyboard comes back with expand on (`crates/app/src/ui/grid/column_factory.rs`), so it can draw wider than its saved width; the manual pass in the backlog | diff --git a/linux/crates/app/src/ui/error_text/driver.rs b/linux/crates/app/src/ui/error_text/driver.rs index cc99066468..acb43cdcb3 100644 --- a/linux/crates/app/src/ui/error_text/driver.rs +++ b/linux/crates/app/src/ui/error_text/driver.rs @@ -1,5 +1,5 @@ use tablepro_core::{ - ConfigError, DriverError, ErrorCategory, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, + BusyReason, ConfigError, DriverError, ErrorCategory, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, TransportError, }; @@ -32,10 +32,15 @@ pub fn driver_message(error: &DriverError) -> String { ErrorCategory::WriteOutcome => write_outcome_message(error), ErrorCategory::File => file_message(error), ErrorCategory::Busy => match error { - DriverError::EditorSessionBusy => { - crate::i18n::gettext("This tab is still running a statement. Stop it or wait for it to finish.") - } - _ => crate::i18n::gettext("The server has no room for another connection right now. Try again shortly."), + DriverError::Busy(reason) => match reason { + BusyReason::NoFreeConnection => { + crate::i18n::gettext("The server has no room for another connection right now. Try again shortly.") + } + BusyReason::FileLocked => crate::i18n::gettext( + "Another program, or an editor tab with an open transaction, holds a lock on this database file. Try again when it has finished.", + ), + }, + _ => crate::i18n::gettext("This tab is still running a statement. Stop it or wait for it to finish."), }, ErrorCategory::Internal => internal_message(error), } diff --git a/linux/crates/app/src/ui/error_text/tests.rs b/linux/crates/app/src/ui/error_text/tests.rs index e185c8165e..3bdb9cf193 100644 --- a/linux/crates/app/src/ui/error_text/tests.rs +++ b/linux/crates/app/src/ui/error_text/tests.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use tablepro_core::credentials::{SecretError, SecretKind}; use tablepro_core::sql_syntax::script::{OpenConstruct, ScriptDiagnostic}; use tablepro_core::{ - ConfigError, DriverError, ErrorCategory, LossPhase, NetworkEndpoint, ReadOnlyRefusal, ServerCode, + BusyReason, ConfigError, DriverError, ErrorCategory, LossPhase, NetworkEndpoint, ReadOnlyRefusal, ServerCode, ServerDiagnostics, SshDestinationPart, SshDestinationProblem, SshFailure, StatementOutcome, TimeoutPhase, TlsFailure, TransportError, }; @@ -69,7 +69,7 @@ fn one_per_category() -> Vec<(ErrorCategory, DriverError)> { path: PathBuf::from("/srv/app.db"), }, ), - (ErrorCategory::Busy, DriverError::Busy), + (ErrorCategory::Busy, DriverError::Busy(BusyReason::NoFreeConnection)), (ErrorCategory::Internal, DriverError::Protocol("bad frame".to_owned())), ] } @@ -195,11 +195,37 @@ fn a_refused_login_is_not_called_a_wrong_password() { #[test] fn busy_and_editor_busy_messages_differ() { assert_ne!( - driver_message(&DriverError::Busy), + driver_message(&DriverError::Busy(BusyReason::NoFreeConnection)), driver_message(&DriverError::EditorSessionBusy) ); } +#[test] +fn a_locked_file_names_the_lock_not_a_full_server() { + let locked = driver_message(&DriverError::Busy(BusyReason::FileLocked)); + let full = driver_message(&DriverError::Busy(BusyReason::NoFreeConnection)); + + assert_eq!( + full, + "The server has no room for another connection right now. Try again shortly." + ); + assert!(locked.contains("lock on this database file"), "{locked}"); + assert!(!locked.contains("server"), "{locked}"); + assert_ne!(locked, driver_message(&DriverError::EditorSessionBusy)); +} + +#[test] +fn a_save_whose_commit_met_a_lock_says_why_and_that_nothing_changed() { + let locked = DriverError::Busy(BusyReason::FileLocked); + let message = driver_message(&DriverError::RolledBack { + statement_index: 1, + source: Box::new(locked.clone()), + }); + + assert!(message.contains(&driver_message(&locked)), "{message}"); + assert!(message.contains("no rows were changed"), "{message}"); +} + #[test] fn file_not_found_names_path() { let message = driver_message(&DriverError::FileNotFound { @@ -447,8 +473,8 @@ fn operation_errors_cancelled_and_closed_differ() { operation_error_message(&OperationError::Closed) ); assert_eq!( - operation_error_message(&OperationError::Driver(DriverError::Busy)), - driver_message(&DriverError::Busy) + operation_error_message(&OperationError::Driver(DriverError::Busy(BusyReason::NoFreeConnection))), + driver_message(&DriverError::Busy(BusyReason::NoFreeConnection)) ); } diff --git a/linux/crates/app/src/workspace/save_outcome.rs b/linux/crates/app/src/workspace/save_outcome.rs index 6c7375b66d..3b756106f6 100644 --- a/linux/crates/app/src/workspace/save_outcome.rs +++ b/linux/crates/app/src/workspace/save_outcome.rs @@ -57,7 +57,9 @@ impl SaveOutcome { mod tests { use tablepro_core::ddl::BuildDdlError; use tablepro_core::dml::BuildSqlError; - use tablepro_core::{CommitToken, LossPhase, ReadOnlyRefusal, StatementOutcome, TimeoutPhase, TransportError}; + use tablepro_core::{ + BusyReason, CommitToken, LossPhase, ReadOnlyRefusal, StatementOutcome, TimeoutPhase, TransportError, + }; use tablepro_session::runtime::TaskPanic; use super::*; @@ -145,7 +147,8 @@ mod tests { SaveOutcome::Unknown(UnknownCause::Interrupted) ); for refused in [ - DriverError::Busy, + DriverError::Busy(BusyReason::NoFreeConnection), + DriverError::Busy(BusyReason::FileLocked), DriverError::server("syntax error"), DriverError::auth(None), DriverError::Transport(TransportError::NameResolution { diff --git a/linux/crates/app/src/workspace/workspace_handle.rs b/linux/crates/app/src/workspace/workspace_handle.rs index 86782a2c11..3fcff8f5e5 100644 --- a/linux/crates/app/src/workspace/workspace_handle.rs +++ b/linux/crates/app/src/workspace/workspace_handle.rs @@ -629,7 +629,7 @@ fn failing_source(plan: &SavePlan, changes: &TrackedChanges, error: &DriverError mod tests { use tablepro_core::edit::CellInput; use tablepro_core::meta::TableRef; - use tablepro_core::{CommitToken, LossPhase, Value}; + use tablepro_core::{BusyReason, CommitToken, LossPhase, Value}; use tablepro_test_support::{CallCounters, ConnectionScript, FakeReply}; use super::*; @@ -850,7 +850,7 @@ mod tests { fn an_error_that_names_no_statement_flashes_nothing() { let (plan, changes, _) = planned(); - assert!(failing_source(&plan, &changes, &DriverError::Busy).is_none()); + assert!(failing_source(&plan, &changes, &DriverError::Busy(BusyReason::NoFreeConnection)).is_none()); let past_the_end = DriverError::RowGuardFailed { statement_index: 7, expected: 1, diff --git a/linux/crates/core/src/busy_reason.rs b/linux/crates/core/src/busy_reason.rs new file mode 100644 index 0000000000..7cd3cdde7d --- /dev/null +++ b/linux/crates/core/src/busy_reason.rs @@ -0,0 +1,22 @@ +use std::fmt; + +/// What was busy. The app acts on both the same way, so this is a +/// value it words rather than a variant of its own. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum BusyReason { + /// The server has no room for another connection, or the pool had + /// no free slot in time. + NoFreeConnection, + /// Another connection to the database file held the lock a + /// statement needed until the lock timeout ran out. + FileLocked, +} + +impl fmt::Display for BusyReason { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + Self::NoFreeConnection => "the server has no room for another connection", + Self::FileLocked => "another connection holds a lock on the database file", + }) + } +} diff --git a/linux/crates/core/src/error.rs b/linux/crates/core/src/error.rs index 4422296175..65a318591e 100644 --- a/linux/crates/core/src/error.rs +++ b/linux/crates/core/src/error.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; use thiserror::Error; +use crate::busy_reason::BusyReason; use crate::commit::CommitToken; use crate::config_error::ConfigError; use crate::error_category::ErrorCategory; @@ -119,8 +120,8 @@ pub enum DriverError { #[error("could not write {}: {kind}", .path.display())] FileIo { path: PathBuf, kind: std::io::ErrorKind }, - #[error("the server has no room for another connection")] - Busy, + #[error("{0}")] + Busy(BusyReason), #[error("the editor session is already running a statement")] EditorSessionBusy, @@ -160,7 +161,7 @@ impl DriverError { | Self::FileAccessDenied { .. } | Self::NotADatabase { .. } | Self::FileIo { .. } => ErrorCategory::File, - Self::Busy | Self::EditorSessionBusy => ErrorCategory::Busy, + Self::Busy(_) | Self::EditorSessionBusy => ErrorCategory::Busy, Self::Config(_) | Self::Unsupported { .. } => ErrorCategory::Configuration, Self::GuardUnsupported { .. } | Self::Decode { .. } | Self::Protocol(_) => ErrorCategory::Internal, } @@ -266,6 +267,17 @@ mod tests { assert_ne!(refused, DriverError::auth(None)); } + #[test] + fn a_locked_file_is_busy_but_not_a_full_server() { + let locked = DriverError::Busy(BusyReason::FileLocked); + let full = DriverError::Busy(BusyReason::NoFreeConnection); + + assert_eq!(locked.category(), ErrorCategory::Busy); + assert_eq!(full.category(), ErrorCategory::Busy); + assert!(!locked.to_string().contains("server"), "{locked}"); + assert_ne!(locked.to_string(), full.to_string()); + } + #[test] fn an_unreadable_column_is_ours_to_fix_not_the_servers() { let error = DriverError::Decode { diff --git a/linux/crates/core/src/lib.rs b/linux/crates/core/src/lib.rs index 20f3ce7c55..d91ccdaf52 100644 --- a/linux/crates/core/src/lib.rs +++ b/linux/crates/core/src/lib.rs @@ -1,5 +1,6 @@ mod auth_mode; pub mod browse; +mod busy_reason; mod call_options; mod capabilities; mod certificate_file_problem; @@ -77,6 +78,7 @@ mod write_ledger; mod write_report; pub use auth_mode::AuthMode; +pub use busy_reason::BusyReason; pub use call_options::CallOptions; pub use capabilities::{ CommitResolutionSupport, DriverCapabilities, EndpointKind, EstimateQuality, ReadOnlyEnforcement, ScriptExecution, diff --git a/linux/crates/drivers/clickhouse/src/errors.rs b/linux/crates/drivers/clickhouse/src/errors.rs index 4848a19e60..ad6168f302 100644 --- a/linux/crates/drivers/clickhouse/src/errors.rs +++ b/linux/crates/drivers/clickhouse/src/errors.rs @@ -3,7 +3,8 @@ use std::io; use clickhouse::error::Error as ClickHouseError; use tablepro_core::{ - DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, TlsFailure, TransportError, + BusyReason, DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, TlsFailure, + TransportError, }; /// Codes the app acts on. Everything else is the server's own answer, @@ -106,7 +107,9 @@ fn server_error(message: &str, context: ErrorContext) -> DriverError { phase: TimeoutPhase::Statement, server_cancelled: true, }, - Some(code::TOO_MANY_SIMULTANEOUS_QUERIES | code::NO_FREE_CONNECTION) => DriverError::Busy, + Some(code::TOO_MANY_SIMULTANEOUS_QUERIES | code::NO_FREE_CONNECTION) => { + DriverError::Busy(BusyReason::NoFreeConnection) + } _ => DriverError::reported(diagnostics), } } diff --git a/linux/crates/drivers/mssql/src/errors.rs b/linux/crates/drivers/mssql/src/errors.rs index 8fb6b29659..556fb593b8 100644 --- a/linux/crates/drivers/mssql/src/errors.rs +++ b/linux/crates/drivers/mssql/src/errors.rs @@ -1,5 +1,5 @@ use deadpool::managed::{PoolError, TimeoutType}; -use tablepro_core::{DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase}; +use tablepro_core::{BusyReason, DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase}; use tiberius::error::Error; use crate::tds_message::TdsMessage; @@ -47,7 +47,7 @@ pub(crate) fn server_error(message: &TdsMessage) -> DriverError { server_cancelled: true, }, // The server's user connection limit. - 17809 => DriverError::Busy, + 17809 => DriverError::Busy(BusyReason::NoFreeConnection), _ => DriverError::reported(diagnostics), } } @@ -81,7 +81,7 @@ pub(crate) fn ends_connection(error: &DriverError) -> bool { /// busy, which is the server having no room rather than a dead link. pub(crate) fn map_pool_error(error: PoolError) -> DriverError { match error { - PoolError::Timeout(TimeoutType::Wait) => DriverError::Busy, + PoolError::Timeout(TimeoutType::Wait) => DriverError::Busy(BusyReason::NoFreeConnection), PoolError::Timeout(TimeoutType::Create) => DriverError::Timeout { phase: TimeoutPhase::Login, server_cancelled: false, @@ -142,7 +142,7 @@ mod tests { ( 17809, "Could not connect because the maximum number of user connections", - |e| matches!(e, DriverError::Busy), + |e| matches!(e, DriverError::Busy(BusyReason::NoFreeConnection)), ), (208, "Invalid object name 't'.", |e| matches!(e, DriverError::Server(_))), (1205, "Transaction was deadlocked", |e| { @@ -157,7 +157,10 @@ mod tests { for (number, text, expected) in cases { let mapped = server_error(&message(number, text)); assert!(expected(&mapped), "{number}: {mapped:?}"); - if !matches!(mapped, DriverError::Timeout { .. } | DriverError::Busy) { + if !matches!( + mapped, + DriverError::Timeout { .. } | DriverError::Busy(BusyReason::NoFreeConnection) + ) { assert_eq!(tds_number(&mapped), Some(number), "{number} kept its number"); } } @@ -225,7 +228,10 @@ mod tests { #[test] fn a_pool_wait_that_runs_out_is_busy() { - assert_eq!(map_pool_error(PoolError::Timeout(TimeoutType::Wait)), DriverError::Busy); + assert_eq!( + map_pool_error(PoolError::Timeout(TimeoutType::Wait)), + DriverError::Busy(BusyReason::NoFreeConnection) + ); assert!(matches!( map_pool_error(PoolError::Closed), DriverError::ConnectionLost { .. } diff --git a/linux/crates/drivers/mysql/src/cancel.rs b/linux/crates/drivers/mysql/src/cancel.rs index 681196081c..17c23d387f 100644 --- a/linux/crates/drivers/mysql/src/cancel.rs +++ b/linux/crates/drivers/mysql/src/cancel.rs @@ -189,7 +189,10 @@ mod tests { assert!(acknowledges_stop(&DriverError::Cancelled, StopProgress::Delivered)); assert!(acknowledges_stop(&timed_out, StopProgress::Delivered)); assert!(!acknowledges_stop(&timed_out, StopProgress::NotSent)); - assert!(!acknowledges_stop(&DriverError::Busy, StopProgress::Delivered)); + assert!(!acknowledges_stop( + &DriverError::Busy(tablepro_core::BusyReason::NoFreeConnection), + StopProgress::Delivered + )); } #[test] diff --git a/linux/crates/drivers/mysql/src/errors.rs b/linux/crates/drivers/mysql/src/errors.rs index e922034a93..bc86482971 100644 --- a/linux/crates/drivers/mysql/src/errors.rs +++ b/linux/crates/drivers/mysql/src/errors.rs @@ -4,8 +4,8 @@ use std::io; use deadpool::managed::{PoolError, TimeoutType}; use sqlx::mysql::MySqlDatabaseError; use tablepro_core::{ - DriverError, LossPhase, NetworkEndpoint, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, TlsFailure, - TransportError, + BusyReason, DriverError, LossPhase, NetworkEndpoint, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, + TlsFailure, TransportError, }; use crate::flavour::MySqlFlavour; @@ -92,7 +92,7 @@ fn map_with(error: &sqlx::Error, flavour: Option) -> DriverError { }, }, sqlx::Error::Tls(tls) => tls_error(tls.as_ref()), - sqlx::Error::PoolTimedOut => DriverError::Busy, + sqlx::Error::PoolTimedOut => DriverError::Busy(BusyReason::NoFreeConnection), sqlx::Error::PoolClosed | sqlx::Error::WorkerCrashed => DriverError::ConnectionLost { during: LossPhase::Idle, }, @@ -166,7 +166,7 @@ pub(crate) fn classify_number(diagnostics: ServerDiagnostics, flavour: Option DriverError::Busy, + TOO_MANY_CONNECTIONS => DriverError::Busy(BusyReason::NoFreeConnection), MYSQL_QUERY_TIMEOUT if mysql => statement_timeout(), MARIADB_STATEMENT_TIMEOUT if mariadb => statement_timeout(), SECURE_TRANSPORT_REQUIRED => requires_tls(diagnostics), @@ -215,7 +215,7 @@ pub(crate) fn server_number(error: &DriverError) -> Option { /// is the server having no room rather than a dead link. pub(crate) fn map_pool_error(error: &PoolError) -> DriverError { match error { - PoolError::Timeout(TimeoutType::Wait) => DriverError::Busy, + PoolError::Timeout(TimeoutType::Wait) => DriverError::Busy(BusyReason::NoFreeConnection), PoolError::Timeout(TimeoutType::Create) => DriverError::Timeout { phase: TimeoutPhase::Login, server_cancelled: false, @@ -273,7 +273,10 @@ mod tests { .. } )); - assert_eq!(classify(TOO_MANY_CONNECTIONS), DriverError::Busy); + assert_eq!( + classify(TOO_MANY_CONNECTIONS), + DriverError::Busy(BusyReason::NoFreeConnection) + ); assert!(matches!(classify(1146), DriverError::Server(_)), "{flavour:?}"); } diff --git a/linux/crates/drivers/postgres/src/errors.rs b/linux/crates/drivers/postgres/src/errors.rs index 821259c33a..3684c3fbc8 100644 --- a/linux/crates/drivers/postgres/src/errors.rs +++ b/linux/crates/drivers/postgres/src/errors.rs @@ -3,7 +3,8 @@ use std::io; use deadpool_postgres::{PoolError, TimeoutType}; use tablepro_core::{ - DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, TlsFailure, TransportError, + BusyReason, DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics, TimeoutPhase, TlsFailure, + TransportError, }; use tokio_postgres::error::{DbError, ErrorPosition}; @@ -82,7 +83,7 @@ pub(crate) fn map_diagnostics(diagnostics: ServerDiagnostics) -> DriverError { phase: TimeoutPhase::LockWait, server_cancelled: true, }, - "53300" => DriverError::Busy, + "53300" => DriverError::Busy(BusyReason::NoFreeConnection), // The server ended the session: an administrator terminated the // backend, or the server is shutting down. "57P01" | "57P02" | "57P03" => DriverError::ConnectionLost { @@ -147,7 +148,7 @@ pub(crate) fn read_only_refusal(error: DriverError) -> DriverError { /// busy, which is the server having no room rather than a dead link. pub(crate) fn map_pool_error(error: &PoolError) -> DriverError { match error { - PoolError::Timeout(TimeoutType::Wait) => DriverError::Busy, + PoolError::Timeout(TimeoutType::Wait) => DriverError::Busy(BusyReason::NoFreeConnection), PoolError::Timeout(TimeoutType::Create) => DriverError::Timeout { phase: TimeoutPhase::Login, server_cancelled: false, @@ -233,7 +234,7 @@ mod tests { ) }), ("53300", "sorry, too many clients already", |e| { - matches!(e, DriverError::Busy) + matches!(e, DriverError::Busy(BusyReason::NoFreeConnection)) }), ("57P01", "terminating connection due to administrator command", |e| { matches!(e, DriverError::ConnectionLost { .. }) @@ -255,7 +256,7 @@ mod tests { }), Some("57014") ); - assert_eq!(sqlstate(&DriverError::Busy), None); + assert_eq!(sqlstate(&DriverError::Busy(BusyReason::NoFreeConnection)), None); } #[test] @@ -281,7 +282,7 @@ mod tests { fn a_pool_wait_that_runs_out_is_busy() { assert_eq!( map_pool_error(&PoolError::Timeout(TimeoutType::Wait)), - DriverError::Busy + DriverError::Busy(BusyReason::NoFreeConnection) ); assert!(matches!( map_pool_error(&PoolError::Closed), diff --git a/linux/crates/drivers/sqlite/src/errors.rs b/linux/crates/drivers/sqlite/src/errors.rs index 833544b233..3e5613bb4c 100644 --- a/linux/crates/drivers/sqlite/src/errors.rs +++ b/linux/crates/drivers/sqlite/src/errors.rs @@ -7,11 +7,10 @@ use std::path::Path; use sqlx::error::DatabaseError; use sqlx::sqlite::SqliteError; -use tablepro_core::{DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics}; +use tablepro_core::{BusyReason, DriverError, LossPhase, ReadOnlyRefusal, ServerCode, ServerDiagnostics}; pub(crate) const SQLITE_ERROR: i32 = 1; pub(crate) const SQLITE_BUSY: i32 = 5; -pub(crate) const SQLITE_LOCKED: i32 = 6; pub(crate) const SQLITE_READONLY: i32 = 8; pub(crate) const SQLITE_INTERRUPT: i32 = 9; pub(crate) const SQLITE_IOERR: i32 = 10; @@ -36,7 +35,7 @@ pub(crate) fn map_error(error: sqlx::Error) -> DriverError { }, // The one workspace connection is held by a longer call, so the // file is busy rather than gone. - sqlx::Error::PoolTimedOut => DriverError::Busy, + sqlx::Error::PoolTimedOut => DriverError::Busy(BusyReason::NoFreeConnection), sqlx::Error::PoolClosed => DriverError::ConnectionLost { during: LossPhase::Idle, }, @@ -79,9 +78,14 @@ pub(crate) fn cannot_open(path: &Path) -> DriverError { } fn server_error(sqlite: &SqliteError) -> DriverError { - let diagnostics = diagnostics_of(sqlite); - match primary_code(sqlite) { - SQLITE_BUSY | SQLITE_LOCKED => DriverError::Busy, + classify(primary_code(sqlite), diagnostics_of(sqlite)) +} + +/// `SQLITE_LOCKED` is the server's own error: without a shared cache it +/// is a conflict inside one connection, which no other program causes. +fn classify(primary: i32, diagnostics: ServerDiagnostics) -> DriverError { + match primary { + SQLITE_BUSY => DriverError::Busy(BusyReason::FileLocked), SQLITE_READONLY => DriverError::ReadOnly(ReadOnlyRefusal::server(diagnostics)), _ => DriverError::reported(diagnostics), } @@ -142,6 +146,36 @@ mod tests { assert_eq!(cannot_open(&present), DriverError::FileAccessDenied { path: present }); } + fn sqlite(extended: i32, message: &str) -> ServerDiagnostics { + ServerDiagnostics::new( + Some(ServerCode::Sqlite { + primary: extended & 0xff, + extended, + }), + message, + ) + } + + #[test] + fn busy_is_a_locked_file_and_a_table_lock_is_the_servers_own_error() { + const SQLITE_BUSY_SNAPSHOT: i32 = 517; + const SQLITE_LOCKED: i32 = 6; + let table_locked = sqlite(SQLITE_LOCKED, "database table is locked"); + + assert_eq!( + classify(SQLITE_BUSY, sqlite(SQLITE_BUSY, "database is locked")), + DriverError::Busy(BusyReason::FileLocked) + ); + assert_eq!( + classify(SQLITE_BUSY, sqlite(SQLITE_BUSY_SNAPSHOT, "database is locked")), + DriverError::Busy(BusyReason::FileLocked) + ); + assert_eq!( + classify(SQLITE_LOCKED, table_locked.clone()), + DriverError::reported(table_locked) + ); + } + #[test] fn an_interrupt_is_recognised_by_its_primary_code() { let interrupted = DriverError::reported(ServerDiagnostics::new( diff --git a/linux/crates/drivers/sqlite/src/interrupt.rs b/linux/crates/drivers/sqlite/src/interrupt.rs index 6f654fdd5a..5ec86195aa 100644 --- a/linux/crates/drivers/sqlite/src/interrupt.rs +++ b/linux/crates/drivers/sqlite/src/interrupt.rs @@ -15,7 +15,7 @@ use std::time::Duration; use async_trait::async_trait; use sqlx::sqlite::SqliteConnection as SqlxConnection; -use tablepro_core::{DriverError, EngineStop, StopDelivery, StopScope}; +use tablepro_core::{BusyReason, DriverError, EngineStop, StopDelivery, StopScope}; use crate::errors::{is_interrupt, map_error}; use crate::lock_wait::LockWait; @@ -53,7 +53,7 @@ impl InterruptFlag { /// statement is interrupted, and one waiting for a lock gives up /// as busy. pub(crate) fn stopped(&self, error: &DriverError) -> bool { - is_interrupt(error) || (self.is_raised() && *error == DriverError::Busy) + is_interrupt(error) || (self.is_raised() && *error == DriverError::Busy(BusyReason::FileLocked)) } /// Make this flag the one the connection's statements answer to, diff --git a/linux/crates/drivers/sqlite/src/write_batch.rs b/linux/crates/drivers/sqlite/src/write_batch.rs index 14e872ec2d..27cb580cef 100644 --- a/linux/crates/drivers/sqlite/src/write_batch.rs +++ b/linux/crates/drivers/sqlite/src/write_batch.rs @@ -3,8 +3,10 @@ //! Every batch starts with BEGIN IMMEDIATE, which takes the write lock //! before the first probe reads anything. A deferred BEGIN would take //! it at the first write instead, and a probe that read first could -//! then fail to upgrade its lock. Contention therefore shows up once, -//! as Busy after the lock timeout, before anything has run. +//! then fail to upgrade its lock. Another writer therefore shows up at +//! BEGIN, before anything has run. In a rollback-journal file a reader +//! elsewhere still holds up the COMMIT, which needs the file to itself, +//! and the batch then rolls back. use std::collections::HashMap; use std::time::Duration; @@ -14,8 +16,8 @@ use sqlx::{AssertSqlSafe, Connection as _, Row}; use tablepro_core::meta::TableRef; use tablepro_core::statement::{GuardedStatement, RowGuard}; use tablepro_core::{ - DriverError, ForeignKeyCheck, ForeignKeyMode, LossPhase, SessionFate, StopReason, StopRequest, TimeoutPhase, - WriteBatch, WriteLedger, WriteReport, + BusyReason, DriverError, ForeignKeyCheck, ForeignKeyMode, LossPhase, SessionFate, StopReason, StopRequest, + TimeoutPhase, WriteBatch, WriteLedger, WriteReport, }; use crate::autocommit::is_autocommit; @@ -253,7 +255,7 @@ impl Write<'_> { /// lowered for the rollback by then, so the request says whether a /// stop came. fn stopped(&self, error: &DriverError) -> Option { - match is_interrupt(error) || *error == DriverError::Busy { + match is_interrupt(error) || *error == DriverError::Busy(BusyReason::FileLocked) { true => self.request.reason().map(stop_cause), false => None, } diff --git a/linux/crates/drivers/sqlite/tests/integration/file_locked.rs b/linux/crates/drivers/sqlite/tests/integration/file_locked.rs new file mode 100644 index 0000000000..af66b6653f --- /dev/null +++ b/linux/crates/drivers/sqlite/tests/integration/file_locked.rs @@ -0,0 +1,253 @@ +//! A database file another program holds locked. Every path that needs +//! the lock fails as `Busy(FileLocked)` once the lock timeout runs out, +//! and a save leaves the file as it was. + +use std::sync::Arc; +use std::time::Duration; + +use sqlx::sqlite::{SqliteConnectOptions, SqliteConnection}; +use sqlx::{Connection as _, Executor as _}; +use tablepro_core::import::CsvImportOptions; +use tablepro_core::meta::TableRef; +use tablepro_core::statement::GuardedStatement; +use tablepro_core::{ + BusyReason, Connection, DriverError, EditorSessionOptions, EditorSessionState, ForeignKeyMode, RowLimit, RunEnd, + Value, WorkspaceSettings, WriteBatch, WriteKind, +}; +use tablepro_test_support::{CsvImportFailure, import_csv}; +use tokio::time::Instant; +use tokio_util::sync::CancellationToken; + +use crate::support::{TestFile, apply, batch, call, connect, query, run, statement}; + +const LOCK_TIMEOUT: Duration = Duration::from_millis(300); + +fn locked() -> DriverError { + DriverError::Busy(BusyReason::FileLocked) +} + +fn settings() -> WorkspaceSettings { + WorkspaceSettings { + lock_timeout: LOCK_TIMEOUT, + ..WorkspaceSettings::workspace(false) + } +} + +async fn open(file: &TestFile) -> Arc { + connect(file.endpoint(), settings()).await.unwrap() +} + +/// A connection of another program's, outside the driver, that has run +/// `statements` and keeps the lock they took. +async fn other_program(file: &TestFile, statements: &[&'static str]) -> SqliteConnection { + let mut holder = SqliteConnection::connect_with(&SqliteConnectOptions::new().filename(&file.path)) + .await + .unwrap(); + for sql in statements { + holder.execute(*sql).await.unwrap(); + } + holder +} + +async fn release(mut holder: SqliteConnection) { + holder.execute("ROLLBACK").await.unwrap(); + holder.close().await.unwrap(); +} + +async fn count(connection: &dyn Connection, sql: &str) -> Value { + query(connection, sql).await.rows[0][0].clone() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_structure_change_into_a_locked_file_creates_nothing() { + let file = TestFile::new().await; + let connection = open(&file).await; + apply(connection.as_ref(), &["CREATE TABLE t (a)"]).await.unwrap(); + let holder = other_program(&file, &["BEGIN IMMEDIATE"]).await; + let rebuild = WriteBatch::new( + WriteKind::SchemaChanges, + TableRef::unqualified("u"), + ["CREATE TABLE u (a)", "INSERT INTO u SELECT a FROM t"] + .iter() + .map(|sql| GuardedStatement::unguarded(statement(sql))) + .collect(), + ForeignKeyMode::SuspendedThenVerified { checks: Vec::new() }, + ); + + let started = Instant::now(); + let changed = connection.write_batch(&rebuild, call()).await; + let waited = started.elapsed(); + release(holder).await; + + assert_eq!(changed.err(), Some(locked())); + assert!(waited >= LOCK_TIMEOUT, "locked after only {waited:?}"); + assert_eq!( + count( + connection.as_ref(), + "SELECT count(*) FROM sqlite_schema WHERE name = 'u'" + ) + .await, + Value::Int(0), + "the table was created" + ); + assert_eq!( + count(connection.as_ref(), "PRAGMA foreign_keys").await, + Value::Int(1), + "foreign keys stayed off" + ); + connection.close().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_reader_elsewhere_holds_up_the_commit_of_a_rollback_journal_file() { + let file = TestFile::new().await; + let connection = open(&file).await; + apply(connection.as_ref(), &["CREATE TABLE t (a)", "INSERT INTO t VALUES (1)"]) + .await + .unwrap(); + let journal = query(connection.as_ref(), "PRAGMA journal_mode").await; + assert_eq!(journal.rows[0][0], Value::Text("delete".to_owned())); + let holder = other_program(&file, &["BEGIN", "SELECT count(*) FROM t"]).await; + + let saved = connection + .write_batch( + &batch( + WriteKind::RowChanges, + &["INSERT INTO t VALUES (2)", "UPDATE t SET a = 3"], + ), + call(), + ) + .await; + release(holder).await; + + assert_eq!( + saved.err(), + Some(DriverError::RolledBack { + statement_index: 2, + source: Box::new(locked()), + }) + ); + let rows = query(connection.as_ref(), "SELECT a FROM t").await; + assert_eq!(rows.rows, vec![vec![Value::Int(1)]], "a write landed in part"); + let next = connection + .write_batch(&batch(WriteKind::RowChanges, &["INSERT INTO t VALUES (4)"]), call()) + .await; + assert!(next.is_ok(), "the next save failed: {next:?}"); + connection.close().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn in_wal_mode_a_reader_elsewhere_does_not_hold_up_a_save() { + let file = TestFile::new().await; + let setup = other_program(&file, &["PRAGMA journal_mode = WAL"]).await; + setup.close().await.unwrap(); + let connection = open(&file).await; + apply(connection.as_ref(), &["CREATE TABLE t (a)", "INSERT INTO t VALUES (1)"]) + .await + .unwrap(); + let holder = other_program(&file, &["BEGIN", "SELECT count(*) FROM t"]).await; + + let saved = connection + .write_batch(&batch(WriteKind::RowChanges, &["INSERT INTO t VALUES (2)"]), call()) + .await; + release(holder).await; + + assert!(saved.is_ok(), "{saved:?}"); + assert_eq!( + count(connection.as_ref(), "SELECT count(*) FROM t").await, + Value::Int(2) + ); + connection.close().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_import_into_a_locked_file_adds_no_rows() { + let file = TestFile::new().await; + let connection = open(&file).await; + apply( + connection.as_ref(), + &["CREATE TABLE people (id INTEGER PRIMARY KEY, name TEXT)"], + ) + .await + .unwrap(); + let holder = other_program(&file, &["BEGIN IMMEDIATE"]).await; + + let imported = import_csv( + connection.as_ref(), + &TableRef::unqualified("people"), + b"id,name\n1,ada\n2,grace\n", + &CsvImportOptions::default(), + call(), + ) + .await; + release(holder).await; + + assert!( + matches!(&imported, Err(CsvImportFailure::Driver(error)) if *error == locked()), + "{imported:?}" + ); + assert_eq!( + count(connection.as_ref(), "SELECT count(*) FROM people").await, + Value::Int(0) + ); + connection.close().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_editor_run_against_a_locked_file_fails_and_the_next_run_works() { + let file = TestFile::new().await; + let connection = open(&file).await; + apply(connection.as_ref(), &["CREATE TABLE t (a)"]).await.unwrap(); + let session = connection + .open_editor_session(EditorSessionOptions::default(), CancellationToken::new()) + .await + .unwrap(); + let holder = other_program(&file, &["BEGIN IMMEDIATE"]).await; + + let (refused, _) = run(session.as_ref(), "INSERT INTO t VALUES (1)").await; + release(holder).await; + let (next, _) = run(session.as_ref(), "INSERT INTO t VALUES (2)").await; + + assert_eq!(refused.end, RunEnd::Failed(locked())); + assert_eq!(next.end, RunEnd::Completed); + assert_eq!(session.state(), EditorSessionState::Ready); + let rows = query(connection.as_ref(), "SELECT a FROM t").await; + assert_eq!(rows.rows, vec![vec![Value::Int(2)]]); + session.close().await; + connection.close().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn reads_of_an_exclusively_locked_file_fail_as_locked() { + let file = TestFile::new().await; + let connection = open(&file).await; + apply(connection.as_ref(), &["CREATE TABLE t (a)"]).await.unwrap(); + let holder = other_program(&file, &["BEGIN EXCLUSIVE"]).await; + + let columns = connection.fetch_columns(&TableRef::unqualified("t"), call()).await; + let page = connection + .query_bound(&statement("SELECT a FROM t"), RowLimit::EDITOR_DEFAULT, call()) + .await; + release(holder).await; + + assert_eq!(columns.err(), Some(locked())); + assert_eq!(page.err(), Some(locked())); + assert_eq!( + count(connection.as_ref(), "SELECT count(*) FROM t").await, + Value::Int(0) + ); + connection.close().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn opening_an_exclusively_locked_file_fails_as_locked() { + let file = TestFile::new().await; + let holder = other_program(&file, &["BEGIN EXCLUSIVE"]).await; + + let opened = connect(file.endpoint(), settings()).await; + release(holder).await; + + assert_eq!(opened.err(), Some(locked())); + let reopened = connect(file.endpoint(), settings()).await.unwrap(); + reopened.close().await.unwrap(); +} diff --git a/linux/crates/drivers/sqlite/tests/integration/main.rs b/linux/crates/drivers/sqlite/tests/integration/main.rs index 05ad354b38..f0bb977342 100644 --- a/linux/crates/drivers/sqlite/tests/integration/main.rs +++ b/linux/crates/drivers/sqlite/tests/integration/main.rs @@ -15,6 +15,8 @@ mod decode; #[cfg(test)] mod dialect; #[cfg(test)] +mod file_locked; +#[cfg(test)] mod lock_wait; #[cfg(test)] mod metadata; diff --git a/linux/crates/drivers/sqlite/tests/integration/sessions.rs b/linux/crates/drivers/sqlite/tests/integration/sessions.rs index 41d5605ec9..9348a2ec9b 100644 --- a/linux/crates/drivers/sqlite/tests/integration/sessions.rs +++ b/linux/crates/drivers/sqlite/tests/integration/sessions.rs @@ -3,8 +3,9 @@ use std::time::Duration; use tablepro_core::{ - CallOptions, DriverError, EditorSessionOptions, EditorSessionState, ErrorCategory, ReadStatement, ResultEvent, - RowLimit, RunEnd, RunOptions, TransactionState, Truncation, UserSql, Value, WorkspaceSettings, WriteKind, + BusyReason, CallOptions, DriverError, EditorSessionOptions, EditorSessionState, ErrorCategory, ReadStatement, + ResultEvent, RowLimit, RunEnd, RunOptions, TransactionState, Truncation, UserSql, Value, WorkspaceSettings, + WriteKind, }; use tokio::time::Instant; use tokio_util::sync::CancellationToken; @@ -228,8 +229,8 @@ async fn session_read_concurrent_with_workspace_write_busy_or_commit_never_hangs let acceptable = match &written { Ok(_) => true, - Err(DriverError::RolledBack { source, .. }) => matches!(**source, DriverError::Busy), - Err(DriverError::Busy) => true, + Err(DriverError::RolledBack { source, .. }) => matches!(**source, DriverError::Busy(BusyReason::FileLocked)), + Err(DriverError::Busy(BusyReason::FileLocked)) => true, Err(_) => false, }; assert!(acceptable, "{written:?}"); diff --git a/linux/crates/drivers/sqlite/tests/integration/write_batch_foreign_keys.rs b/linux/crates/drivers/sqlite/tests/integration/write_batch_foreign_keys.rs index e1a0a85f07..68037fae99 100644 --- a/linux/crates/drivers/sqlite/tests/integration/write_batch_foreign_keys.rs +++ b/linux/crates/drivers/sqlite/tests/integration/write_batch_foreign_keys.rs @@ -8,8 +8,8 @@ use sqlx::{Connection as _, Executor as _}; use tablepro_core::meta::TableRef; use tablepro_core::statement::GuardedStatement; use tablepro_core::{ - Connection, DriverError, ForeignKeyCheck, ForeignKeyMode, ServerCode, Value, WorkspaceSettings, WriteBatch, - WriteKind, + BusyReason, Connection, DriverError, ForeignKeyCheck, ForeignKeyMode, ServerCode, Value, WorkspaceSettings, + WriteBatch, WriteKind, }; use tokio::time::Instant; @@ -180,7 +180,7 @@ async fn write_batch_takes_write_lock_before_probe() { holder.execute("ROLLBACK").await.unwrap(); holder.close().await.unwrap(); - assert!(matches!(outcome, Err(DriverError::Busy)), "{outcome:?}"); + assert_eq!(outcome.err(), Some(DriverError::Busy(BusyReason::FileLocked))); assert!(waited >= Duration::from_millis(250), "busy after only {waited:?}"); let rows = query(connection.as_ref(), "SELECT a FROM t ORDER BY id").await; assert_eq!( diff --git a/linux/crates/session/src/health/failure_cause.rs b/linux/crates/session/src/health/failure_cause.rs index eba7ce9cdd..6814afa8cf 100644 --- a/linux/crates/session/src/health/failure_cause.rs +++ b/linux/crates/session/src/health/failure_cause.rs @@ -48,7 +48,7 @@ impl FailureCause { #[cfg(test)] mod tests { - use tablepro_core::{ConfigError, LossPhase}; + use tablepro_core::{BusyReason, ConfigError, LossPhase}; use super::*; @@ -95,6 +95,6 @@ mod tests { }), None ); - assert_eq!(FailureCause::of(&DriverError::Busy), None); + assert_eq!(FailureCause::of(&DriverError::Busy(BusyReason::NoFreeConnection)), None); } } diff --git a/linux/crates/session/src/health/health_monitor.rs b/linux/crates/session/src/health/health_monitor.rs index 9edaca04f3..5af62acf57 100644 --- a/linux/crates/session/src/health/health_monitor.rs +++ b/linux/crates/session/src/health/health_monitor.rs @@ -282,7 +282,7 @@ async fn discard_late(pending: PendingSession) { mod tests { use std::sync::Arc; - use tablepro_core::{DriverError, LivenessPolicy, LossPhase, NetworkEndpoint, TransportError}; + use tablepro_core::{BusyReason, DriverError, LivenessPolicy, LossPhase, NetworkEndpoint, TransportError}; use tablepro_test_support::{CallCounters, ConnectionScript, FakeConnection, FakePrompter, FakeReply}; use super::*; @@ -686,7 +686,9 @@ mod tests { #[tokio::test(start_paused = true)] async fn busy_probe_keeps_online() { let mut script = ConnectionScript::default(); - script.probe.always(FakeReply::err(DriverError::Busy)); + script + .probe + .always(FakeReply::err(DriverError::Busy(BusyReason::NoFreeConnection))); let started = start(script, FakeConnector::new([])); tokio::time::sleep(HealthPolicy::PROBE_INTERVAL * 5).await; diff --git a/linux/crates/session/src/health/probe_outcome.rs b/linux/crates/session/src/health/probe_outcome.rs index 8522370378..256f278c9c 100644 --- a/linux/crates/session/src/health/probe_outcome.rs +++ b/linux/crates/session/src/health/probe_outcome.rs @@ -44,7 +44,7 @@ impl ProbeOutcome { #[cfg(test)] mod tests { - use tablepro_core::{LossPhase, NetworkEndpoint, SshFailure, TimeoutPhase, TransportError}; + use tablepro_core::{BusyReason, LossPhase, NetworkEndpoint, SshFailure, TimeoutPhase, TransportError}; use super::*; @@ -63,7 +63,10 @@ mod tests { })); assert_eq!(ProbeOutcome::classify(Ok(())), ProbeOutcome::Healthy); - assert_eq!(ProbeOutcome::classify(Err(DriverError::Busy)), ProbeOutcome::Busy); + assert_eq!( + ProbeOutcome::classify(Err(DriverError::Busy(BusyReason::NoFreeConnection))), + ProbeOutcome::Busy + ); assert_eq!( ProbeOutcome::classify(Err(DriverError::Timeout { phase: TimeoutPhase::Probe, diff --git a/linux/crates/session/src/operation/replay_rule.rs b/linux/crates/session/src/operation/replay_rule.rs index 446611626d..f339a7bef4 100644 --- a/linux/crates/session/src/operation/replay_rule.rs +++ b/linux/crates/session/src/operation/replay_rule.rs @@ -30,7 +30,7 @@ impl ReplayRule { #[cfg(test)] mod tests { use tablepro_core::{ - ConfigError, DriverError, LossPhase, NetworkEndpoint, TimeoutPhase, TransportError, WriteKind, + BusyReason, ConfigError, DriverError, LossPhase, NetworkEndpoint, TimeoutPhase, TransportError, WriteKind, }; use super::*; @@ -47,7 +47,7 @@ mod tests { server_cancelled: true, }, ErrorCategory::Server => DriverError::server("relation does not exist"), - ErrorCategory::Busy => DriverError::Busy, + ErrorCategory::Busy => DriverError::Busy(BusyReason::NoFreeConnection), ErrorCategory::Authentication => DriverError::auth(None), ErrorCategory::Configuration => DriverError::Config(ConfigError::DatabaseRequired), _ => DriverError::Protocol("other".to_owned()), diff --git a/linux/crates/session/tests/session_postgres.rs b/linux/crates/session/tests/session_postgres.rs index 8ab79beddb..33ccbf959e 100644 --- a/linux/crates/session/tests/session_postgres.rs +++ b/linux/crates/session/tests/session_postgres.rs @@ -13,9 +13,9 @@ use secrecy::SecretString; use tablepro_core::credentials::CredentialInteraction; use tablepro_core::statement::{GuardedStatement, Statement, StatementBuilder}; use tablepro_core::{ - CallOptions, ConnectEnvironment, ConnectOptions, ConnectTarget, Connection, Credentials, DatabaseDriver, - DriverError, ErrorCategory, ForeignKeyMode, LivenessPolicy, NetworkEndpoint, NetworkTarget, RowLimit, TableRef, - TlsConfig, Value, WorkspaceSettings, WriteBatch, WriteKind, + BusyReason, CallOptions, ConnectEnvironment, ConnectOptions, ConnectTarget, Connection, Credentials, + DatabaseDriver, DriverError, ErrorCategory, ForeignKeyMode, LivenessPolicy, NetworkEndpoint, NetworkTarget, + RowLimit, TableRef, TlsConfig, Value, WorkspaceSettings, WriteBatch, WriteKind, }; use tablepro_driver_postgres::{PgDriver, WORKSPACE_POOL_MAX_SIZE}; use tablepro_net::DirectTcpTransport; @@ -265,7 +265,10 @@ async fn saturated_pool_operation_busy_probe_ok_stays_online() { session.signal(MonitorSignal::ProbeRequested { generation }); tokio::time::sleep(Duration::from_secs(2)).await; - assert_eq!(crowded, Err(OperationError::Driver(DriverError::Busy))); + assert_eq!( + crowded, + Err(OperationError::Driver(DriverError::Busy(BusyReason::NoFreeConnection))) + ); assert_eq!(*session.health().borrow(), SessionHealth::Online); assert_eq!( *session.link_changes().borrow(), diff --git a/linux/crates/test-support/src/conformance/cases/pool.rs b/linux/crates/test-support/src/conformance/cases/pool.rs index e32b80cd77..8d9a034785 100644 --- a/linux/crates/test-support/src/conformance/cases/pool.rs +++ b/linux/crates/test-support/src/conformance/cases/pool.rs @@ -126,7 +126,7 @@ async fn prove_saturated(connection: &dyn Connection, size: usize) -> Result<(), #[cfg(test)] mod tests { - use tablepro_core::{DriverError, QueryResult}; + use tablepro_core::{BusyReason, DriverError, QueryResult}; use super::*; use crate::conformance::fake_engine::{FakeEngine, FakeScenario}; @@ -162,7 +162,7 @@ mod tests { .when_sql_contains("BLOCK", holding()) .always(FakeReply::Bounded { after: test_liveness().pool_wait, - outcome: Err(DriverError::Busy), + outcome: Err(DriverError::Busy(BusyReason::NoFreeConnection)), }); script } diff --git a/linux/crates/test-support/src/fake_connection.rs b/linux/crates/test-support/src/fake_connection.rs index 6550851ecc..bbfea689f8 100644 --- a/linux/crates/test-support/src/fake_connection.rs +++ b/linux/crates/test-support/src/fake_connection.rs @@ -465,10 +465,10 @@ mod tests { use std::time::Duration; use futures::future::BoxFuture; - use tablepro_core::LossPhase; use tablepro_core::meta::ObjectKind; use tablepro_core::statement::{Statement, StatementBuilder}; use tablepro_core::value::Value; + use tablepro_core::{BusyReason, LossPhase}; use tokio::time::Instant; use super::*; @@ -494,7 +494,7 @@ mod tests { name: "items".to_owned(), kind: ObjectKind::Table, }])) - .once(FakeReply::err(DriverError::Busy)); + .once(FakeReply::err(DriverError::Busy(BusyReason::NoFreeConnection))); script.probe.always(FakeReply::ok(())); let connection = FakeConnection::new(script); let counters = connection.counters(); @@ -506,7 +506,7 @@ mod tests { assert_eq!(listed.len(), 1); assert!(matches!( connection.list_objects(&ObjectScope::DefaultSchema, unbounded()).await, - Err(DriverError::Busy) + Err(DriverError::Busy(BusyReason::NoFreeConnection)) )); assert!(matches!( connection.list_objects(&ObjectScope::DefaultSchema, unbounded()).await, @@ -655,9 +655,9 @@ mod tests { #[tokio::test] async fn a_stopped_with_reply_answers_its_outcome_only_once_cancelled() { let mut script = ConnectionScript::default(); - script - .query_bound - .always(FakeReply::StoppedWith(Err(DriverError::Busy))); + script.query_bound.always(FakeReply::StoppedWith(Err(DriverError::Busy( + BusyReason::NoFreeConnection, + )))); let connection = Arc::new(FakeConnection::new(script)); let cancel = CancellationToken::new(); @@ -678,7 +678,10 @@ mod tests { cancel.cancel(); - assert!(matches!(waiting.await.unwrap(), Err(DriverError::Busy))); + assert!(matches!( + waiting.await.unwrap(), + Err(DriverError::Busy(BusyReason::NoFreeConnection)) + )); assert!(observed[0].is_cancelled(), "the call was handed another token"); assert!(connection.observed_cancels("write_batch").is_empty()); } diff --git a/linux/docs/drivers/clickhouse.md b/linux/docs/drivers/clickhouse.md index 73d577fc08..e8d00ce3a8 100644 --- a/linux/docs/drivers/clickhouse.md +++ b/linux/docs/drivers/clickhouse.md @@ -59,7 +59,7 @@ A stop is `KILL QUERY WHERE query_id = ? ASYNC` on the control client. Each quer | 159 | `Timeout { phase: Statement, server_cancelled: true }` | | 164, 392 | `ReadOnly(ServerRejected)` | | 192, 193, 194, 516; 497 on the first request | `Auth` | -| 202, 203 | `Busy` | +| 202, 203 | `Busy(NoFreeConnection)` | | 373 on an editor run | `EditorSessionBusy` | | 372 on an editor run | `ConnectionLost`: the server let the session expire | diff --git a/linux/docs/drivers/mysql.md b/linux/docs/drivers/mysql.md index 8d68c81b68..ac0f480856 100644 --- a/linux/docs/drivers/mysql.md +++ b/linux/docs/drivers/mysql.md @@ -82,7 +82,7 @@ A stop is `KILL QUERY ` on the control connection, which reaches | 1205 | `Timeout { phase: LockWait }` | | 1792 | `ReadOnly(ServerRejected)` | | 1045 | `Auth` | -| 1040 | `Busy` | +| 1040 | `Busy(NoFreeConnection)` | | 3159 | `Tls { failure: ServerRequiresTls }` | | 1053, 1927 (MariaDB), 4031 (MySQL) | `ConnectionLost` | diff --git a/linux/docs/drivers/postgresql.md b/linux/docs/drivers/postgresql.md index b62636d40c..68ae4c947c 100644 --- a/linux/docs/drivers/postgresql.md +++ b/linux/docs/drivers/postgresql.md @@ -54,7 +54,7 @@ A stop is PostgreSQL's cancel request (`CancelToken::cancel_query_raw`) over a n | 25006 | `ReadOnly(ServerRejected)` | | 28P01 | `Auth` | | 28000, as when pg_hba.conf has no entry for the login | `LoginRefused` | -| 53300 | `Busy` | +| 53300 | `Busy(NoFreeConnection)` | | 57P01, 57P02, 57P03 | `ConnectionLost` | The server translates its message through `lc_messages`, so the driver tells its own statement timeout from another session's cancel by the time that passed, not by the text. diff --git a/linux/docs/drivers/sql-server.md b/linux/docs/drivers/sql-server.md index b4e4100ece..1906a2d872 100644 --- a/linux/docs/drivers/sql-server.md +++ b/linux/docs/drivers/sql-server.md @@ -86,7 +86,7 @@ SQL Server has no session statement timeout. A workspace call's deadline is the | 1222 | `Timeout { phase: LockWait }` | | 3906 | `ReadOnly(ServerRejected)` | | 18456, 18452, 18486, 18487, 18488 | `Auth` | -| 17809 | `Busy` | +| 17809 | `Busy(NoFreeConnection)` | | A GSSAPI failure | `IntegratedAuth` | ## Writes diff --git a/linux/docs/drivers/sqlite.md b/linux/docs/drivers/sqlite.md index 86807ec2d3..c9c8422281 100644 --- a/linux/docs/drivers/sqlite.md +++ b/linux/docs/drivers/sqlite.md @@ -11,7 +11,7 @@ Every connection to the file opens with the same options: - The path is a file name, never a URI, so `?` and `%41` in it are part of the name. - A missing file is `FileNotFound`; the driver never creates one by accident (`create_if_missing` is off). - A read-only connection opens the file with `SQLITE_OPEN_READONLY`. -- `busy_timeout` is the workspace's lock timeout, 10 s: how long SQLite waits for another connection's lock before it reports `Busy`. A call replaces it with a busy handler that waits as long and also ends at a stop ([Stops and timeouts](#stops-and-timeouts)). +- `busy_timeout` is the workspace's lock timeout, 10 s: how long SQLite waits for another connection's lock before it reports `Busy(FileLocked)`. A call replaces it with a busy handler that waits as long and also ends at a stop ([Stops and timeouts](#stops-and-timeouts)). - `foreign_keys = ON`, and `trusted_schema = OFF`, so a view or trigger in a file from elsewhere cannot run SQL functions with side effects. - The statement cache is off, because sqlx keeps a statement's columns from its first prepare and SQLite prepares it again with more columns after an `ALTER TABLE`. - sqlx's statement logging is off. @@ -46,18 +46,19 @@ The health probe runs `SELECT 1` on the workspace connection when it is free. Wh SQLite has no statement timeout and no out-of-band cancel. Each call installs a fresh flag, and a progress handler checks it every 1,000 virtual-machine instructions; when the flag is up the statement ends with `SQLITE_INTERRUPT`. The flag names the call, not the connection, so a stop that arrives late cannot reach the next call: `StopDelivery::OutOfBand(StopScope::Statement)`. `sqlite3_interrupt` is not used, because sqlx's handle to the connection waits for the worker thread to finish its current command, which is the statement the stop was meant for. -A statement waiting for another connection's lock runs no instructions, so each call also installs a busy handler in place of `busy_timeout`. It sleeps the steps SQLite's own would, up to the lock timeout, and gives up as `Busy` once the flag is up, which reads as the stop. +A statement waiting for another connection's lock runs no instructions, so each call also installs a busy handler in place of `busy_timeout`. It sleeps the steps SQLite's own would, up to the lock timeout, and gives up as `Busy(FileLocked)` once the flag is up, which reads as the stop. The workspace statement timeout (60 s) becomes a deadline the flag enforces. The flag is lowered before a rollback runs, so the rollback is never interrupted by the stop that caused it. | SQLite answer | Reported as | |---|---| | 9 (`SQLITE_INTERRUPT`) after our stop | `Cancelled`, or `Timeout { server_cancelled: true }` when the deadline asked for it | -| 5, 6 (`SQLITE_BUSY`, `SQLITE_LOCKED`) | `Busy` | +| 5 (`SQLITE_BUSY`) | `Busy(FileLocked)`: another connection held the lock past the lock timeout | +| 6 (`SQLITE_LOCKED`) | `Server`: without a shared cache it is a conflict inside one connection, which no other program causes | | 8 (`SQLITE_READONLY`) | `ReadOnly(ServerRejected)` | | 14 (`SQLITE_CANTOPEN`) at open | `FileNotFound`, or `FileAccessDenied` when the file is there | | 26 (`SQLITE_NOTADB`) at open | `NotADatabase` | -| The workspace connection not free within the pool wait | `Busy` | +| The workspace connection not free within the pool wait | `Busy(NoFreeConnection)` | ## Read-only @@ -65,7 +66,12 @@ The workspace statement timeout (60 s) becomes a deadline the flag enforces. The ## Writes -`WriteAtomicity::Transactional` for row and schema changes. A save is one transaction that starts with `BEGIN IMMEDIATE`, which takes the write lock before the first probe reads anything: contention shows up once, as `Busy` after the lock timeout, before anything has run. +`WriteAtomicity::Transactional` for row and schema changes. A save is one transaction that starts with `BEGIN IMMEDIATE`, which takes the write lock before the first probe reads anything. Grid saves, structure changes and imports all go through it. Another connection holding the lock shows up in one of two places, each as `Busy(FileLocked)` after the lock timeout, and the file keeps none of the save: + +- At `BEGIN IMMEDIATE`, when another connection is writing: the save fails as `Busy(FileLocked)` before anything has run. +- At `COMMIT`, in a file in rollback-journal mode (the default; not WAL), when another connection has a read transaction open: the COMMIT needs the file to itself, so the save fails as `RolledBack` with `Busy(FileLocked)` as its cause. In WAL mode a reader does not hold up a save. + +An editor run that writes, a read while another connection holds the file exclusively (in rollback-journal mode, while it commits), and opening such a file wait the same way and fail as `Busy(FileLocked)`. `tests/integration/file_locked.rs` covers each path. SQLite is the one engine that can suspend foreign keys for a batch (`ForeignKeyMode::SuspendedThenVerified`), which a table rebuild needs for its copy: diff --git a/linux/docs/error-handling.md b/linux/docs/error-handling.md index 8ac17cb714..a2e08903fd 100644 --- a/linux/docs/error-handling.md +++ b/linux/docs/error-handling.md @@ -33,7 +33,7 @@ Errors are typed enums, built with `thiserror` wherever they cross a crate bound | `RolledBack`, `RowGuardFailed`, `CommitOutcomeUnknown`, `PartiallyApplied` | What a failed save left in the database ([adding-drivers.md](adding-drivers.md#5-writes-and-the-writeledger)). | | `GuardUnsupported` | The engine reports no row count, so a guarded write cannot run. | | `FileNotFound`, `FileAccessDenied`, `NotADatabase`, `FileIo` | A file engine's file. | -| `Busy` | The server has no room for another connection, or the pool had no free slot in time. | +| `Busy(BusyReason)` | Something was busy: `NoFreeConnection` (the server has no room for another connection, or the pool had no free slot in time) or `FileLocked` (another connection held the database file's lock past the lock timeout). | | `EditorSessionBusy` | The tab's session is already running a statement. | | `Config(ConfigError)` | The settings cannot be used: a bad host, port, path, TLS option, auth mode or missing database name. | | `Decode { column, detail }` | The driver could not read a value. A bug on our side, not the server's. The detail is for the log. | @@ -64,7 +64,7 @@ Rules: | `ConnectionLost` | `ConnectionLost` | Says the connection closed. The session reconnects on a backoff. | | `WriteOutcome` | `RolledBack`, `RowGuardFailed`, `CommitOutcomeUnknown`, `PartiallyApplied` | Says what is in the database now. Only `RolledBack` and `RowGuardFailed` may say nothing changed. | | `File` | the four file variants | Names the path. A connect that failed on the file is not retried. | -| `Busy` | `Busy`, `EditorSessionBusy` | Says to try again shortly, or to wait for the tab's statement. | +| `Busy` | `Busy`, `EditorSessionBusy` | Says to try again shortly, that another program or tab holds the file's lock, or to wait for the tab's statement. | | `Internal` | `GuardUnsupported`, `Decode`, `Protocol`; `SshFailure::Protocol` | Says what the driver could not do: check a save, read a value, read the reply. | ## `ServerDiagnostics`