alkstore/tasks/pg-fix-open-path.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
pg-fix-open-path	Fix `max_size: 0` open hang + DSN options override (review 002 Finding 3 + options note)	completed		single	low	component	implementation	wave-4-fixes postgres-engine open

Description

Two open-path fixes in alkstore-postgres/src/store.rs (open_store), from the wave-4 general review:

Finding 3 (MEDIUM, live-proven): PgOpts { max_size: 0 } hangs open forever. Site: store.rs:162-165 + the bootstrap pool.get() at store.rs:177 — deadpool (0.13/0.14) does not validate max_size and defaults to no timeouts, so a max_size: 0 pool can never grant a checkout: open blocks forever at the bootstrap checkout (no BuildError, no default pool timeouts) instead of failing typed. Probe (removed): open with max_size: 0 timed out at 5 s, never returned.

Fix shape (the review's): validate max_size > 0 in open_store — typed Error::Database before any round trip (the engine-wide validate-at-entry posture; the source-chain detail should say what was rejected, e.g. "max_size must be positive"). The alternative shape (configure deadpool timeouts so the hang surfaces as an error) is weaker — a timeout-configured hang still burns the consumer's time; prefer the guard. Do both only if trivial.

Minor note (same file, same surface): store.rs:152-156 — pool_cfg.options(...) replaces any options= present in the consumer's DSN (tokio-postgres's setter semantics) — a silent config override. Preferred fix: append to the DSN's existing options string (tokio-postgres Config::get_options() reads what the parse carried) so the engine's synchronous_commit SET rides alongside whatever the consumer configured. If appending proves impractical, document at the site and in the crate docs that engine-configured options win — but append is the honest default (the consumer's DSN options were there first).

Tests (against the harness server): max_size: 0 fails typed and promptly (assert the error; the test's own structure bounds the hang — a regression re-hangs and fails the test by timeout); a DSN carrying pre-existing options= plus open succeeds and synchronous_commit is still observable via SHOW on both settings (the append path doesn't break the engine's SET); the existing opts-flow tests stay green.

Acceptance Criteria

• 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 — 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) — n/a: append landed
• Existing open/opts tests stay green
• cargo test -p alkstore-postgres (harness server), clippy -D warnings, fmt clean; gates green server-less

References

• docs/reviews/002-wave-4-general-review.md §Finding 3 + §Minor notes (the options bullet)
• docs/architecture/engine-postgres.md (Connection architecture)
• docs/architecture/decisions/008-contract-v1-pinning.md §6 (config split)
• tasks/pg-engine-open-opts.md (the open task of record; Notes carry the connect-options SET mechanics this task extends)

Notes

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

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.