Repository navigation
test(infra): verify PostgreSQL and Redis for real, and fix the rate-limit bypass it exposed - #44
Merged
Conversation
…stence - Add tests verifying Agent.run loads prior session conversation turns from memory - Add test verifying POST /runs preserves multi-turn context with same session_id
Combine an ephemeral SessionStore (RedisMemoryStore) for conversation history with a durable run repository (PostgresMemoryStore) behind a single MemoryStore implementation, and wire it in the API dependency graph. Agent.run now loads prior turns for a session_id, re-inserts the system prompt when absent, and persists the final assistant turn.
The limiter read the window cardinality with ZCARD and inserted the new member with ZADD in two separate round trips, with an await between them. Concurrent requests therefore all observed the same pre-insert count and were admitted together: 19 of 20 simultaneous requests passed against a limit of 5. Any client could exceed its quota by sending requests in parallel. Replace the split read/write with a single Lua script so trimming the expired entries, counting and inserting happen as one indivisible operation inside Redis. A non-atomic path is retained only for deployments that refuse scripting, where a best-effort limit is preferable to refusing all traffic. fakeredis only implements EVAL when a Lua runtime is present, so add lupa as a dev dependency; without it the hermetic rate-limit tests would keep exercising a different code path than production.
The suite previously exercised PostgresMemoryStore, PostgresTraceSink and RedisMemoryStore only through in-memory SQLite and fakeredis, so nothing covered the behaviour those stand-ins actively differ on. SQLite renders uuid/json/timestamptz as CHAR/TEXT and ignores foreign keys unless PRAGMA foreign_keys=ON; fakeredis is a pure-Python reimplementation that never exercises the hiredis codec or genuine TTL expiry. Add tests/conftest.py with live fixtures plus tests/real covering: - native uuid, json and timestamptz columns and asyncpg parameter binding - ON DELETE CASCADE and rejection of orphan steps by a real foreign key - Alembic migrations applied through the real PostgreSQL dialect - real TTL expiry, the hiredis codec and corrupt-payload degradation - the Lua-backed rate limiter, including the concurrency regression that motivated 3ea6b93 - a full POST /runs round trip on the real FastAPI lifespan, asserting the rows land in runs and steps and the session key lands in Redis The tier is gated on AGENTKIT_REAL_INFRA=1 so the default pytest run stays hermetic, and it targets a dedicated test database: it truncates runs and steps and deletes only session:*/ratelimit:* keys.
get_db_session only yielded inside `if factory is not None`, so with no session factory configured the async generator completed without yielding. FastAPI's dependency solver treats that as a failure and raises an opaque `generator didn't yield` RuntimeError, which the generic handler then reported as an HTTP 500. Raise ServiceUnavailableError instead, mapped to 503, so an unconfigured database is reported as such. Depend on get_session_factory rather than resolving the Request by hand, which also lets callers override it. While here, have the lifespan reuse the create_engine and create_session_factory helpers in agentkit.db.session instead of rebuilding the engine and sessionmaker inline, removing a second, divergent copy of that construction.
…containers The workflow declared PostgreSQL 16 and Redis 7 service containers and set DATABASE_TEST_URL and REDIS_URL, but no test read either variable; the containers were provisioned and then left idle. Split the single test job into a hermetic matrix (Python 3.11 and 3.12, coverage gated) and a dedicated job that runs the real_infra tier against real services, using Redis database 15 to keep it clear of the hermetic job's namespace. Also stop defaulting the compose api service to the known test key ak_test_key_12345. The stack now refuses to start without an explicit API_KEYS, so a published key cannot reach a reachable instance, and .env.example documents the requirement alongside the optional real-tier connection URLs.
The demo is the artifact a reviewer is most likely to run, and it used in-memory SQLite, InMemoryTraceSink and no memory store at all, so it exercised neither of the two stores the project exists to demonstrate. Default to the configured DATABASE_URL and REDIS_URL, composing the run tier over PostgresMemoryStore and the session tier over RedisMemoryStore, and pass the stores and a session id to the Agent. A Redis outage now degrades only the session tier instead of failing the run. --offline keeps the previous fully hermetic behaviour, and --db-url still overrides the target database.
The README stated the CI matrix ran "with live PostgreSQL 16 and Redis 7 service containers", which overstated what was verified: the containers existed but no test connected to them. PROGRESS.md made the same claim for the compose work while having only checked `docker compose config`, which validates syntax rather than a running stack. Describe the two tiers honestly, including the opt-in AGENTKIT_REAL_INFRA=1 command, what the live tier actually asserts, and the requirement to point it at a dedicated database. Update the compose quickstart for the new API_KEYS requirement, note that the demo now persists by default, and record the reasoning as ADR-005.
Using ${API_KEYS:?} made the compose file fail interpolation for every
command when the variable was unset, so `down`, `ps` and `logs` all broke
for anyone who had not exported a key.
Switch to ${API_KEYS:-}. The api service still refuses to run without a
key, because an empty value fails Settings validation at startup and the
container exits naming API_KEYS, but the rest of the lifecycle keeps
working. Verified both paths against the built stack: with no key the api
service exits on `API_KEYS must not be empty` while postgres and redis
stay healthy; with a key all three come up healthy and serve requests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The suite claimed to verify PostgreSQL and Redis but never touched either.
PostgresMemoryStore,PostgresTraceSinkandRedisMemoryStorewere exercised exclusively against in-memory SQLite andfakeredis, and CI declared PostgreSQL 16 / Redis 7 service containers that no test connected to. This adds an opt-in live tier, and fixes a rate-limit bypass the live tier exposed.What was mocked instead of real
uuid/json/timestamptz, FK enforcement, orON DELETE CASCADE(SQLite ignores FKs withoutPRAGMA foreign_keys=ON)fakeredisonly — no hiredis codec, no real TTL expirymigrations/env.pyFakeLLMClientRate limiter admitted 19 of 20 concurrent requests against a limit of 5
agentkit/api/ratelimit.pyread the window cardinality withZCARDand inserted the member withZADDin two round trips with anawaitbetween them. Concurrent requests all observed the same pre-insert count and were admitted together, so any client could exceed its quota by sending requests in parallel. Replaced with a single Lua script performing trim, count and insert as one indivisible operation. Verified by reverting the fix against the new test:admitted 19 == 5fails; with the fix, exactly 5 of 20 pass.Also fixed
get_db_sessionreturned without yielding when no session factory was configured, producing an opaquegenerator didn't yieldRuntimeError surfaced as HTTP 500. Now raisesServiceUnavailableErrormapped to 503.docker-compose.ymldefaultedAPI_KEYStoak_test_key_12345. The stack now refuses to start without an explicit value.examples/demo.pypersisted nothing — in-memory SQLite,InMemoryTraceSink, no memory store. Now persists run and step records to real PostgreSQL and Redis by default;--offlinekeeps the hermetic path.docs/PROGRESS.md, with the rationale in ADR-005.Verification
Beyond the test suites, the stack was built and exercised end to end:
docker compose up -d --buildbrings up api/postgres/redis healthy with themigrationsone-shot exiting 0;/healthreports both dependencies healthy;/toolsserves all five built-ins;POST /runsreturned a run whose row was confirmed inruns, whose step rows appeared insteps, and whose session key appeared in Redis. The demo was run against real services and itsruns/steps/Redis writes confirmed by direct SQL.Two findings left out of scope
Found while exercising the running stack, unrelated to the test-infrastructure gap:
LLM_MODEL=gemini-2.5-flashreturns404 NOT_FOUND — This model models/gemini-2.5-flash is no longer available to new users. Changing the default does not fix this, because...gemini-3.8-flashthe request fails400 INVALID_ARGUMENT — Function call is missing a thought_signature in functionCall parts.agentkit/llm/gemini.pybuildsPart.from_function_call(line 80) and readspart.function_call(line 145) but never captures or forwardsthought_signature. Every run includes tools, so all runs fail against current Gemini models. This needs a real adapter change and warrants its own PR.Review notes
e2a73ab(tiered session persistence) and3c32f75were pre-existing uncommitted/unshipped work onfeat/session-memory-persistence, committed first so each commit in this stack is independently green.runs/stepsand deletes onlysession:*/ratelimit:*keys, so it needs a dedicated test database. CI points it atagentkit_testand Redis db 15.lupawas added as a dev dependency: fakeredis only implementsEVALwith a Lua runtime, so without it the hermetic rate-limit tests would keep exercising a different code path than production.