✨ feat(db): read SQLAlchemy engine pool configuration from config file

[itisnotyourenv]

Pull Request

Description

Move hardcoded SQLAlchemy engine pool parameters from create_engine function defaults into PostgresConfig, allowing per-environment tuning via YAML config files. Also adds pool_pre_ping (enabled by default) and echo_pool support.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code quality improvement
• Configuration change

Testing

• Tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have tested integration scenarios if applicable

Code Quality

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have made corresponding changes to the documentation if needed
• Pre-commit hooks pass (run pre-commit run --all-files)
• Ruff linting passes (ruff check src/ tests/)
• Ruff formatting passes (ruff format src/ tests/ --check)

Related Issues

Fixes #73

Additional Notes

Changes

File	Change
src/infrastructure/config.py	Added 6 pool fields to PostgresConfig with sensible defaults
src/infrastructure/db/factory.py	Simplified create_engine to read all pool params from config
config-example.yaml	Added commented-out pool settings for discoverability
tests/unit/infrastructure/test_config.py	Added test_pool_defaults and test_pool_custom_values

New config fields (all optional with defaults)

Field	Default	Purpose
pool_size	30	Max persistent connections in pool
pool_timeout	30	Seconds to wait for a connection from pool
pool_recycle	3600	Seconds before a connection is recycled
max_overflow	20	Max additional connections beyond pool_size
pool_pre_ping	true	Verify connections are alive before use
echo_pool	false	Log pool checkout/checkin events

Non-breaking: all new fields have defaults matching the previous hardcoded values.

---

Checklist for Reviewers:

• Code follows project conventions and style guidelines
• Changes are well-tested with appropriate test coverage
• Documentation is updated if necessary
• Security implications have been considered
• Performance implications have been considered

🤖 Generated with Claude Code

[claude]

PR Review: feat(db): read SQLAlchemy engine pool configuration from config file

Overall assessment: 🔶 Request Changes

The change is well-motivated and cleanly structured. The PR description is thorough and the diff is easy to follow. However, there are a few issues that should be addressed before merging.

---

Critical Issues

1. No validation on the new pool fields

PostgresConfig adds six new int/bool fields but validates none of them. Invalid values will be passed silently to SQLAlchemy and surface as confusing engine-level errors at startup (or worse, at runtime when the pool is first used).

Problematic values that should be rejected:

• pool_size < 1 — SQLAlchemy raises ArgumentError: Pool size must be at least 1
• max_overflow < 0 — SQLAlchemy raises ArgumentError
• pool_timeout <= 0 — leads to immediate TimeoutError on every checkout
• pool_recycle <= 0 — meaningless or causes SQLAlchemy to recycle every connection immediately

The existing port_validator pattern in the same class shows how to do this. Each field should get a @field_validator (or a single multi-field validator) that bounds the value to a sane range.

2. echo_pool: bool silently drops SQLAlchemy's "debug" option

SQLAlchemy's create_async_engine accepts echo_pool: bool | Literal["debug"]. The "debug" level logs additional detail (pool state on every checkout/checkin) and is genuinely useful during development. By typing the field as bool, Pydantic will coerce echo_pool: "debug" from YAML to True without warning, and the user has no way to enable the "debug" level.

Consider:

from typing import Literal, Union
echo_pool: Union[bool, Literal["debug"]] = False

---

Suggestions

3. Default pool sizes may be surprising for a template

The chosen defaults (pool_size=30, max_overflow=20) give a maximum of 50 connections per engine instance. SQLAlchemy's own built-in defaults are pool_size=5, max_overflow=10. With PostgreSQL's default max_connections=100, running two instances of this service would already exhaust the connection limit.

These were the previous hardcoded values, so keeping them preserves existing behaviour — but since this PR makes the values configurable and visible, it's a good moment to reconsider whether 30+20 is the right default for a template project that most users will run locally or in small deployments. At minimum, a comment in config-example.yaml warning about this would be helpful.

4. Missing test: pool fields round-tripped through YAML

TestLoadConfig.test_load_valid_config_file verifies that echo loads correctly from YAML, but there is no equivalent test for the new pool fields. A test passing pool_size: 10 etc. in the YAML fixture and asserting the config object picks them up would complete the coverage story.

5. create_engine signature change is a breaking API change

The PR description says "Non-breaking: all new fields have defaults", which is true for the config model. But the create_engine(db_config, pool_size=X, ...) function signature also changed — keyword arguments pool_size, pool_timeout, pool_recycle, and max_overflow were removed. Any external caller passing those kwargs would get a TypeError. Worth a one-line mention in the PR description under "Breaking Changes".

(Within this repo the only callers are create_pool and DBProvider.get_engine, neither of which used those kwargs — so in practice there is no regression here.)

---

Minor Notes

• The port_validator (pre-existing, not introduced here) accepts port=0 despite the error message saying "between 1 and 65535". Out of scope for this PR but worth a separate fix.
• echo_pool in config-example.yaml is listed after pool_pre_ping, which matches the field order in PostgresConfig. Good consistency.
• The two new test methods are clear and straightforward. Naming is consistent with the rest of the test class.

---

Summary

#	Severity	Description
1	Critical	Add @field_validator guards for pool_size, pool_timeout, pool_recycle, max_overflow
2	Critical	Widen echo_pool type to Union[bool, Literal["debug"]]
3	Suggestion	Revisit default pool sizes or add a warning comment in config-example.yaml
4	Suggestion	Add a TestLoadConfig test case that round-trips the new pool fields through YAML
5	Minor	Note the create_engine function signature change in the PR description

🤖 Generated with Claude Code