Replace ateredis with atepg

[Jet Chiang (supreme-gg-gg)]

Summary

A follow up to #640 where we introduced PostgreSQL as an alternative storage backend, selected conditionally in ateapi.

• Deleted ateredis, its tests, and its dependencies
• Removed Redis backend selection and configuration so ateapi always connects to Postgres
• Replaced Valkey resources with Postgres in the standard and Kind deployment paths and simplified install script
• Replaced miniredis fixtures with isolated Postgres testcontainers and added centralized helpers for seeding resources
• Renamed Redis-specific debug flush command to backend-neutral debug-clear-store in CLI
• Updated comments and docs where applicable

Benchmarking

Extensive benchmarking have been performed to evaluate Redis vs Postgres, and results can be found in these two documents:

• https://docs.google.com/document/d/10K0wB6aTeFkJCL4HN3NbLJCdFGoLYdhIcqkFnqFHkKc/edit?usp=sharing
• https://docs.google.com/document/d/12-ko_BFHcBo_nJkx9f4B7zMbiiWKC2saGhMhZG3aQ-s/edit?usp=sharing

[Eitan Yarmush (EItanya)]

Very large PR, but so far this is my main comment

[Joe Betz (jpbetz)]

The startup DDL is CREATE TABLE IF NOT EXISTS and is not going to be sufficient long term.

Reliable detection of the DDL state to ensure compatibility with the binary will be needed early, and a upgrade mechanism will need to be introduced to allow for future schema updates. Fix now or track with an issue?

[Haven Xia (HavenXia)]

+1 for the upgrade mechanism, ideally it should keep the schema compatible during a pod rolling window, where old and new binaries are hitting the same database at the same time, so migrations follow an expand and contract pattern.

[Julian Gutierrez Oschmann (juli4n)]

+1 See #901

[Joe Betz (jpbetz)]

Is the switch to requiring the DB be available on the first request on startup intentional here? The redis path was more tolerant of blips.

[Jet Chiang (supreme-gg-gg)]

Not an intentional design, will add retry like redis

[Joe Betz (jpbetz)]

This connects as the postgres superuser with no password, and the server side is trust auth, so ateapi runs all its DDL and DML with full privileges. Recommend sorting out the app roles now (or very soon) so that you build on least-privilege instead of having to figure it out later.

[Jet Chiang (supreme-gg-gg)]

I think it's best to define this alongside migration design in #901, opened an issue to track it in #997

[Joe Betz (jpbetz)]

I can see the value having this had for ease-of-testing for redis.
But I checked and there is are no server side role restrictions. And this is now deleting persistent state.

Is there an appropriately prioritized tracking issue to figure out how to remove or replace this before v1.0?

[Jet Chiang (supreme-gg-gg)]

opened #999 to track this

[Joe Betz (jpbetz)]

Is there a reason why creating a actor_template_versions without a parent is allowed? Is it to support out-of-order creation? The difference between how the referential integrity of this reference and object->namespace (which is FK projected) is what made me wonder what the rationale is here.

[Joe Betz (jpbetz)]

I'm concerned that the proto payload can go over the 8000 byte NOTIFY limit with not well defined handling of the resulting behavior.

This appears to be a real problem because a larg-ish label set would make every worker write for that pool fail (scheduling included).

[Jet Chiang (supreme-gg-gg)]

I was aware of this size limit, but we will also be moving away from the pg_notify mechanism #934 which will bypass this issue

[Joe Betz (jpbetz)]

One thing to keep an eye on now that Postgres backs the hot path. ResumeActor takes the actor lease before the already-running check, so every routed cold check does a lease upsert plus delete on a tiny table.

The leases have potential to cause a lot of churn. The leases can also become permanently orphaned. Since this is persistent state, cleanup is needed (open issue or address now?)

[Joe Betz (jpbetz)]

Thanks for the careful work on the schema design.

At a high level, the main concerns I have with the PostgreSQL implementation are below. Please consider either addressing in this PR or opening issues to ensure this all gets resolved:

1. Superuser for everything. Let's move to a least-privilege app role before this gets real exposure.
2. No schema evolution or mismatch detection. The schema is applied as CREATE TABLE IF NOT EXISTS on every startup. Later, if the schema were to be updated, there is nothing to detect drift for a fresh install vs. upgraded clusters.
3. Pessimistic locking. The read-modify-write paths hold SELECT ... FOR UPDATE row locks while app code runs between statements means that rows are locked for however long it takes for the server to run the code between statements. I'm concerned the scalability implications of having the lock duration paired with the go code execution.
4. Found worrying number of concurrency issues around pre-condition checks that can race with other DB updates. I pointed out quite a few of them and did a AI run to search for them as well, but they are difficult to find. Recommend adding significantly more tests that interleave operations that can interact.
5. Not enough unexpected input test cases. PageSize cases I found are a good example. Recommend going through all user inputs on all operations and checking for bad inputs.
6. LISTEN/NOTIFY robustness. Allowing undecodable events to be silently dropped instead of closing the channel, so consumers can't tell they have a gap, is unsafe. Recommend hardening the channel code for bad event shapes. Better to fail with a clear error than to silently drop data on a channel.
7. Object lifecycles. Orphaned leases is a good example of where transitioning to a persistent store surfaces issues that are not safe to ignore. In a persistent store like PostgreSQL, the orphaned data needs to get cleaned up.

[Joe Betz (jpbetz)]

An AI scan found these cases and they both look legitimate:

Case 1: the worker vanishes mid-claim, client is told the actor doesn't exist.

1. Router asks to resume actor X. Scheduler picks worker W from the in-memory cache.
2. W's pod dies. The syncer notices and deletes W's row from the store.
3. The resume tries to claim W. The CAS UpdateWorker finds no row and returns ErrNotFound. That error is about the worker.
4. The retry loop only retries version conflicts, so it gives up and passes the error up as is (workflow_resume.go#L323).
5. The ResumeActor handler sees ErrNotFound and assumes it means the actor (actor.go#L410-L418). Client gets NotFound 'Actor X not found'.
6. The router treats NotFound as final, so the request dies with a 404 for an actor that exists. The right outcome was to retry with another worker.

Case 2: heavy contention becomes a permanent 500.

1. Resume tries to claim a worker. Another resume gets there first, the CAS loses with ErrVersionConflict.
2. The loop retries with a fresh worker, 5 attempts total.
3. Pool is busy, all 5 lose the same way.
4. The backoff helper then returns its own generic 'timed out waiting for the condition' error, not the last conflict.
5. Nothing recognizes that error, so it surfaces as Internal, which the router won't retry.

[Joe Betz (jpbetz)]

This handler only checks ErrAlreadyExists (actor.go#L103-L109), so the ErrFailedPrecondition error falls through and the client gets Internal instead of 'Atespace not found'.

Recommend a test that deletes the atespace concurrently with the create.

[Joe Betz (jpbetz)]

Same problem as mentioned for CreateActor. If you create a tag for a snapshot that doesn't exist and you get ErrNotFound from the pre-check. But if the snapshot is deleted after the pre-check and before the insert, the FK violation comes back as ErrFailedPrecondition instead.

Generally we need to not rely on pre-checks to guard an operation if it can race with other operations.

[Joe Betz (jpbetz)]

Please add tests for invalid page tokens. These should not result in internal errors.

[Joe Betz (jpbetz)]

Looks like it's possible to panic the server by setting PageSize to 0 or negative. Please fix and add tests.

[Joe Betz (jpbetz)]

I decided to also do a quick scan for unindexed queries.

Postgres automatically indexes the referenced side of a foreign key but not the referencing side, and there's no index on actor_snapshot_tags (snapshot_atespace, snapshot_name) so the queries against are full scans.

[Jet Chiang (supreme-gg-gg)]

Good catch for this, thanks! I missed the usage of this index in the FK trigger when deleting snapshots

[Eitan Yarmush (EItanya)]

Thanks for the careful work on the schema design.

At a high level, the main concerns I have with the PostgreSQL implementation are below. Please consider either addressing in this PR or opening issues to ensure this all gets resolved:

1. Superuser for everything. Let's move to a least-privilege app role before this gets real exposure.
2. No schema evolution or mismatch detection. The schema is applied as CREATE TABLE IF NOT EXISTS on every startup. Later, if the schema were to be updated, there is nothing to detect drift for a fresh install vs. upgraded clusters.
3. Pessimistic locking. The read-modify-write paths hold SELECT ... FOR UPDATE row locks while app code runs between statements means that rows are locked for however long it takes for the server to run the code between statements. I'm concerned the scalability implications of having the lock duration paired with the go code execution.
4. Found worrying number of concurrency issues around pre-condition checks that can race with other DB updates. I pointed out quite a few of them and did a AI run to search for them as well, but they are difficult to find. Recommend adding significantly more tests that interleave operations that can interact.
5. Not enough unexpected input test cases. PageSize cases I found are a good example. Recommend going through all user inputs on all operations and checking for bad inputs.
6. LISTEN/NOTIFY robustness. Allowing undecodable events to be silently dropped instead of closing the channel, so consumers can't tell they have a gap, is unsafe. Recommend hardening the channel code for bad event shapes. Better to fail with a clear error than to silently drop data on a channel.
7. Object lifecycles. Orphaned leases is a good example of where transitioning to a persistent store surfaces issues that are not safe to ignore. In a persistent store like PostgreSQL, the orphaned data needs to get cleaned up.

First off, thanks so much for the detailed review! I'll run through these one at a time so we can try and see if we're on the same page.

1. 10000%. I think the idea was to merge something quickly, but this needs to be locked down before release
2. 10000%. We are aware on this and someone from my team should be able to pick this up soon. Define postresql schema update strategy #901
3. Totally agree with this takeaway. I also think this falls a bit under schema design, from what I can tell we could avoid the client side code by moving the types directly into the schema, but that has downsides as well. FOR UPDATE NO KEY however seems like an obvious win here.
4. Again totally agree. I think a big part of this is the awkward split between the service layer and the DB layer. This feels like an artifact of redis code and something we should absolutely address.
5. Yup!
6. We're actually moving away from LISTEN/NOTIFY anyway, see atepg: deliver worker watch events via a transactional change feed #934. But we should definitely make sure to handle the cases you mentioned.
7. Absolutely

[Julian Gutierrez Oschmann (juli4n)]

Nit: We shouldn't talk about PostgreSQL here as it's not really relevant information for the API client.

[Joe Betz (jpbetz)]

Julian Gutierrez Oschmann (@juli4n) Benjamin Elder (@BenTheElder) For scale, a couple things to consider:

• Don't use pessimistic concurrency at all. Switch to optimistic concurrency to shift the concurrency cost off of the DB.
• Drop the DB level UNIQUE constraint on UID and leave it to the server to set this correctly. This lets you avoid paying the latency cost and DB load of a unique check for each write. If you ensure that the server mints UIDs, you still have the guarantee.
• Drop the cascading deletes and consider dropping the FKs entirely. Also, don't rely on the FKs to provide fast lookup. FK's are not indexed automatically, So the RESTRICT check on delete results in a full scan, as written.

[Jet Chiang (supreme-gg-gg)]

Joe Betz (@jpbetz), thank you for the detailed review! I addressed the review comments in a separate PR #1023, to keep those changes separate from the larger changes in this PR, which do not modify the PostgreSQL implementation itself.

I also switched the PostgreSQL store to optimistic concurrency, removed the UNIQUE constraints on server-generated UIDs, and added the missing indexes on referencing columns, as you've correctly pointed out.

Drop the cascading deletes and consider dropping the FKs entirely.

I don’t believe there are currently any cascading deletes; the foreign keys use ON DELETE RESTRICT.

Dropping the foreign keys would be a significant semantic tradeoff. I’m not aware of an alternative that preserves the current race-free referential-integrity guarantees without introducing additional locking or transactional coordination. As you noted in several comments, application-level existence checks alone cannot provide the same guarantees because they can race with concurrent deletion.

[Joe Betz (jpbetz)]

Thanks Jet Chiang (@supreme-gg-gg)! I'll follow along on the subsequent PR.