diff --git a/alkstore-postgres/src/opts.rs b/alkstore-postgres/src/opts.rs index 7185712..b47932e 100644 --- a/alkstore-postgres/src/opts.rs +++ b/alkstore-postgres/src/opts.rs @@ -50,7 +50,10 @@ pub struct PgOpts { /// queries, claims, and all non-transactional work ride these. /// Default [`DEFAULT_MAX_SIZE`] = 8 (the deployment matrix's /// budget line; the listener adds +1 per LISTEN-ing process - /// *outside* the pool — sizing rule `max_size + 1`). + /// *outside* the pool — sizing rule `max_size + 1`). A value of + /// `0` fails [`open`](crate::open) with [`Error::Database`] + /// immediately (validated at entry — no round trip; deadpool + /// cannot grant checkouts from an empty pool). pub max_size: usize, /// The per-session `synchronous_commit` durability knob, wired as /// a connect-options `-c synchronous_commit=…` on every pooled diff --git a/alkstore-postgres/src/store.rs b/alkstore-postgres/src/store.rs index 3baf833..59f4deb 100644 --- a/alkstore-postgres/src/store.rs +++ b/alkstore-postgres/src/store.rs @@ -143,17 +143,38 @@ pub async fn open(config: &str, opts: PgOpts) -> alkstore::Result } pub(crate) async fn open_store(config: &str, opts: PgOpts) -> alkstore::Result { + // Validate at entry (the engine-wide posture): a `max_size: 0` + // pool can never grant a checkout, and deadpool (0.13/0.14) does + // not validate the size or apply default timeouts — without this + // guard `open` would hang forever at the bootstrap checkout + // instead of failing typed. This fires before any round trip. + if opts.max_size == 0 { + return Err(database_error(format!( + "max_size must be positive, got {}", + opts.max_size + ))); + } + let cfg: tokio_postgres::Config = config.parse().map_err(pg_error)?; // Per-session durability knob: the POC's connect-options SET // mechanics (`-c synchronous_commit=…` on every pooled // connection; POC-verified — pool.rs's `set_sync_commit_session` - // probe shape). + // probe shape). The engine's option is *appended* to whatever + // options the consumer's DSN carried (tokio-postgres's setter + // replaces — reading the parse-carried value via `get_options()` + // first keeps the consumer's `options=` alive alongside; postgres + // parses repeated `-c` flags, so both ride the connection). let mut pool_cfg = cfg.clone(); - pool_cfg.options(format!( + let engine_opt = format!( "-c synchronous_commit={}", if opts.synchronous_commit { "on" } else { "off" } - )); + ); + let merged = match pool_cfg.get_options() { + Some(existing) => format!("{existing} {engine_opt}"), + None => engine_opt, + }; + pool_cfg.options(merged); let mgr_cfg = ManagerConfig { recycling_method: RecyclingMethod::Fast, diff --git a/alkstore-postgres/src/store/open_tests.rs b/alkstore-postgres/src/store/open_tests.rs index c1ba633..39b6142 100644 --- a/alkstore-postgres/src/store/open_tests.rs +++ b/alkstore-postgres/src/store/open_tests.rs @@ -701,6 +701,95 @@ async fn store_trait_methods_are_wired() { drop_schema(&admin, &schema).await; } +/// Acceptance (review 002 Finding 3): `max_size: 0` fails `open` with +/// the typed `Database` error *before any round trip* — promptly, not +/// the deadpool hang (a `max_size: 0` pool can never grant a checkout +/// and deadpool 0.13/0.14 neither validates it nor defaults timeouts). +/// The test's inner timeout bounds the hang: a regression re-hangs and +/// fails here by timeout. This needs no server — the guard fires at +/// entry, before any connection attempt (its "before any round trip" +/// property pinned by running against an unreachable endpoint). +#[tokio::test] +async fn open_fails_database_promptly_on_zero_max_size() { + let dsn = harness_dsn().unwrap_or_else(unreachable_dsn); + let schema = instance_namer("zero")(); + let opts = PgOpts { + schema, + max_size: 0, + ..PgOpts::default() + }; + let result = tokio::time::timeout(Duration::from_secs(2), open_store(&dsn, opts)).await; + assert!( + result.is_ok(), + "open with max_size: 0 must fail promptly — \ + an inner timeout tripped, the deadpool hang regressed" + ); + let err = result.unwrap().unwrap_err(); + match err { + alkstore::Error::Database(source) => { + let detail = source.to_string(); + assert!( + detail.contains("max_size") && detail.contains("positive"), + "the source chain says what was rejected, got: {detail}" + ); + } + other => panic!("max_size: 0 must fail typed Database, got {other:?}"), + } +} + +/// Acceptance (review 002 options note): the consumer DSN's `options=` +/// survive `open` — the engine's `synchronous_commit` `-c` SET is +/// *appended*, not substituted (tokio-postgres's setter replaces; the +/// parse-carried value is read back and preserved). Both knob values +/// ride every pooled connection: the consumer's +/// (`statement_timeout` via `SHOW`) and the engine's +/// (`synchronous_commit` via `SHOW` on both settings). +#[tokio::test(flavor = "multi_thread")] +async fn consumer_dsn_options_survive_alongside_the_engine_set() { + let Some(dsn) = harness_dsn() else { + eprintln!("skip: no harness server"); + return; + }; + let consumer_dsn = format!("{dsn} options='-c statement_timeout=30000'"); + + for (sync_commit, expected) in [(true, "on"), (false, "off")] { + let schema = instance_namer("dsnops")(); + let store = open_store( + &consumer_dsn, + PgOpts { + schema: schema.to_string(), + synchronous_commit: sync_commit, + ..PgOpts::default() + }, + ) + .await + .unwrap_or_else(|e| panic!("open with a DSN carrying options must succeed: {e}")); + let conn = store.pool().get().await.unwrap(); + + let row = conn.query_one("SHOW statement_timeout", &[]).await.unwrap(); + let timeout: String = row.get(0); + assert_eq!( + timeout, "30s", + "the consumer's DSN options must survive open, got: {timeout}" + ); + + let row = conn + .query_one("SHOW synchronous_commit", &[]) + .await + .unwrap(); + let value: String = row.get(0); + assert_eq!( + value, expected, + "the engine's SET must still ride the appended options, got: {value}" + ); + + drop(conn); + store.close(); + let admin = harness_client().await.unwrap(); + drop_schema(&admin, &schema).await; + } +} + /// The error-mapping helpers exist and are the documented reuse point: /// a mapped error is `Error::Database` with the source chain preserved /// (the mapping trio the mechanism tasks reuse; the io-Error string diff --git a/tasks/pg-fix-open-path.md b/tasks/pg-fix-open-path.md index 3df0607..272c8e0 100644 --- a/tasks/pg-fix-open-path.md +++ b/tasks/pg-fix-open-path.md @@ -1,7 +1,7 @@ --- id: pg-fix-open-path name: Fix `max_size: 0` open hang + DSN options override (review 002 Finding 3 + options note) -status: pending +status: completed depends_on: [] scope: single risk: low @@ -53,16 +53,16 @@ opts-flow tests stay green. ## Acceptance Criteria -- [ ] `max_size: 0` fails `open` with a typed `Database` error before +- [x] `max_size: 0` fails `open` with a typed `Database` error before any round trip — pinned by test (prompt failure, no hang) -- [ ] The consumer DSN's `options=` survive: appended, not replaced — +- [x] The consumer DSN's `options=` survive: appended, not replaced — pinned by test (DSN with options + engine SET coexist, `SHOW` observable) - [ ] If append proved impractical instead: the engine-wins semantics documented at the site and in crate docs (record the decision in - Notes) -- [ ] Existing open/opts tests stay green -- [ ] `cargo test -p alkstore-postgres` (harness server), clippy + Notes) — *n/a: append landed* +- [x] Existing open/opts tests stay green +- [x] `cargo test -p alkstore-postgres` (harness server), clippy `-D warnings`, fmt clean; gates green server-less ## References @@ -77,8 +77,67 @@ opts-flow tests stay green. ## Notes -> To be filled by implementation agent +Decisions of record made while implementing (the description didn't +pin them): + +- **Both fix shapes' primary option landed (the guard; no deadpool + timeout config added)**: `open_store` rejects `opts.max_size == 0` + at entry — `database_error("max_size must be positive, got 0")` + (the value rides the message; the source chain says what was + rejected) — *before* the config parse and any round trip. The + timeout-configured-hang alternative was not taken (the review ranked + it weaker; not trivially worth doing both — deadpool 0.13/0.14 has + no default pool timeouts to lean on and the guard is total). +- **Append is practical and landed** (the task's preferred fix; the + n/a acceptance row): tokio-postgres 0.7.18's + `Config::get_options() -> Option<&str>` returns the raw options + string the parse (or a prior programmatic `options()`) carried, so + `open_store` reads it, appends + ` -c synchronous_commit={on|off}`, and calls the setter — the + consumer's `options=` ride every pooled connection alongside the + engine's SET. Postgres parses repeated `-c` flags in order, so if + the consumer's own options set `synchronous_commit`, the engine's + later `-c` wins *for that knob* (it is the knob `PgOpts` owns) and + every other consumer setting lives untouched. The listener path + (`cfg.clone()` + `application_name` only) is unchanged — the SET is + a pool-connection concern. Documented at the site and on + `PgOpts::max_size` (0 rejected immediately) in `opts.rs`. +- **Tests**: `open_fails_database_promptly_on_zero_max_size` pins the + guard — typed `Database`, source chain containing "max_size" + + "positive", bounded by an inner `tokio::time::timeout(2s)` so a + regression re-hangs and fails here by timeout; it runs server-less + too (unreachable-DSN fallback — the guard fires before any + connection attempt, which is exactly the "before any round trip" + property being pinned). `consumer_dsn_options_survive_alongside_the_engine_set` + pins the append: DSN `options='-c statement_timeout=30000'` → open + succeeds, `SHOW statement_timeout` = `30s` (consumer's survived) + and `SHOW synchronous_commit` = on/off per `PgOpts` (engine's SET + rides) — both knob values asserted for both settings, against the + harness server. +- **The `PgOpts::max_size` doc on `opts.rs` now states the 0 + rejection** (the constructor's validation is part of the option's + contract now that it is typed-failed at open). ## Summary -> To be filled on completion \ No newline at end of file +Both open-path fixes landed in `alkstore-postgres/src/store.rs` +(`open_store`). Finding 3: a `max_size > 0` entry guard fires before +the config parse and any round trip — `max_size: 0` now fails `open` +with the typed `Error::Database` ("max_size must be positive, got 0") +instead of hanging forever at the bootstrap checkout (deadpool +0.13/0.14 neither validates `max_size` nor defaults timeouts). Minor +note: the engine's connect-options SET (`-c synchronous_commit=…`) is +now *appended* to the parse-carried `options=` string (read back via +`tokio_postgres::Config::get_options()`) rather than replacing it — +the consumer's DSN options survive on every pooled connection +alongside the engine's knob; repeated `-c` flags are +order-processed by postgres so the engine still owns +`synchronous_commit` if it collides. Pinned by two new tests in +`src/store/open_tests.rs` (the zero-guard test with a bounded-hang +timeout structure, server-less-capable; the append test asserting +both `SHOW statement_timeout` and `SHOW synchronous_commit` across +both knob settings against the harness server). Verified: 118/118 pg +lib tests + contract-suite/schema suites green against the harness +(pglo-poc :15432), workspace `cargo test` green server-less (283 +tests, pg skipping per convention), `cargo clippy --all-targets -- -D +warnings` and `cargo fmt --check` clean workspace-wide. \ No newline at end of file