Harden PostgreSQL concurrency, integrity, and error handling by supreme-gg-gg · Pull Request #1023 · agent-substrate/substrate

Jet Chiang (supreme-gg-gg) · 2026-08-17T21:57:50Z

This PR makes improvements to the Postgres store by addressing review comments on #940 and optimizing Postgres updates as suggested in #988.

Replaces Postgres row-locking updates for actors, templates, and snapshot tags with bounded optimistic concurrency control using UID/version CAS checks.
Strengthens relational integrity with missing FKs and indexes, and replace snapshot-tag creation pre-check with FK
Removes unnecessary UID uniqueness constraints
Standardize pagination and validation across stores, map malformed tokens and invalid sizes to appropriate gRPC errors
Improves actor resume race handling when workers disappear or concurrent assignments exhaust their retry budget
Improves Postgres reliability with startup retries, expired-lease cleanup, worker-watch reconnection after malformed msg
Expand Postgres, Redis, Contract, and Control API tests to cover new constraints, error semantics, concurrency behaviour

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

…in list ops, and replace snapshot tag creation precheck with fk Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

…tor RPC Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

…o fixes potential data loss on corrupt worker notifications Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

Tim Hockin (thockin)

I skimmed - this is not my expertise area, but a few thoughts

Tim Hockin (thockin) · 2026-08-18T01:14:41Z

        REFERENCES atespaces(name) ON DELETE RESTRICT,
    name      text NOT NULL,
-    uid       text NOT NULL UNIQUE,
+    uid       text NOT NULL,


Should we include UID in every resource table? Not everything is updateable now, but that tends to not last, and we want consistent precondition semantics for every resource. This way we do not need to back-populate them when we inevitably need them later.

I see atespaces and actor_snapshots do not have UID

actor_snapshots should be immutable, so it should not need UID for update? I can add version and UID to atespaces in case it will become updatable

Immutability tends to erode, unless there's a clear reason not to we should probably just be consistent?

Tim Hockin (thockin) · 2026-08-18T01:27:59Z


+// updateMaxAttempts bounds how many times a read-modify-write is retried after
+// its optimistic uid/version check loses to a concurrent writer.
+const updateMaxAttempts = 5


Why do we retry at all if there are preconditions specified on input? If I asked for version==6 and I find DB has version 7, no retries will ever fix it. If we support non-precondition updates, this could matter.

The current update interface does support non-precondition updates, so the retry loop intends to support unpinned mutations which wish to reapply the change to the latest stored value, there is also a test in atepg_test.go that exhibits this behaviour. You're right about version-pinned updates, they will return with ErrVersionConflict after a failed CAS.

I just double checked that all ateapi callers are pinned using WithPrecondition, so we might want to remove the retry loop and make the update interface always pinned. However, this seems to me to be partially reversing #763 which replaced expectedVersion with a transactional mutate closure.

Julian Gutierrez Oschmann (@juli4n) is the goal of #763 to support retryable, unpinned read-modify-writes?

If we end up keeping the retry loop, recommend adding a small amount of jittered backoff. Without it this is a relatively hot loop that expires quickly. More bang for the buck if you spread the requests over a bit more time.

Julian's proposal for update is:

Whole-object updates, require preconditions now

Maybe whole-object updates without preconditions later

Probably patch-updates without preconditions later

I'm fine to keep the loop now as long as we only loop when there are no preconditions in the user input. Note that (I think?) a patch is still:

user calls patch with no preconditions loop read old value call mutate() submit to storage with preconditions if success; break loop

?

Tim Hockin (thockin) · 2026-08-18T01:28:42Z

+// its optimistic uid/version check loses to a concurrent writer.
+const updateMaxAttempts = 5
+
+func validateMetadataProjection(resource string, metadata *ateapipb.ResourceMetadata, uid string, version int64) error {


Is Projection a term I don't know or did you mean Precondition?

Here "projection" refers to the version/uid columns "projected" from the stored proto, which is different from precondition which refers to caller provided expectations. I agree it's a bit unfamiliar, I'll change it to something like validateProtoMetadataMatchesColumns, which is more literal.

Thanks! A comment would suffice.

Joe Betz (jpbetz)

Thanks for getting this all addressed, this all looks like it's moving in the right direction to me. A few minor comments and I saw Tim Hockin (@thockin) had some comments, after that this looks ready to me.

Joe Betz (jpbetz) · 2026-08-18T00:30:07Z

 func (p *Persistence) AcquireLock(ctx context.Context, key string) (*store.Lock, error) {
 	ttl := p.lockTTL
 	token := uuid.NewString()
+	if err := p.cleanupExpiredLeases(ctx); err != nil {


Recommend opening an issue to move this to a dedicated cleanup task for the long term. It's not super urgent, but it's worth tracking and circling back on.

Opened #1040 to track

Joe Betz (jpbetz) · 2026-08-18T00:30:07Z

+	parent := resources.ActorTemplateRefFromObjectRef(atv.GetActorTemplate())
+	exists, err := s.ActorTemplateExists(ctx, parent)
+	if err != nil {
+		return nil, fmt.Errorf("while checking actor template parent %s: %w", parent, err)
+	}
+	if !exists {
+		return nil, store.ErrFailedPrecondition


Consider adding a comment that is best-effort but orphans here are possible.

This was referenced Aug 17, 2026

Replace ateredis with atepg #940

Open

Update on postgres impl is inefficient #988

Open

Jet Chiang (supreme-gg-gg) marked this pull request as ready for review August 17, 2026 22:43

Jet Chiang (supreme-gg-gg) added 9 commits August 17, 2026 21:29

postgres retry

994b1a5

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

fix: fk constraints on atv reference to at, improve pagetoken errors …

51ca338

…in list ops, and replace snapshot tag creation precheck with fk Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

pagination errors and handlers

81da1a1

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

fix: handle ErrFailedPrecondition when atespace not found in CreateAc…

b9a360b

…tor RPC Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

fix race conditions in worker resume flow

5244514

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

add relevant changes to ateredis for store contract tests

2525d6c

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

update ateapi tests and store tests

aa290f4

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

remove unique on uid and add missing fk index

bf929c7

Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

change pessimistic locking to use optimistic concurrency control, als…

f547a2f

…o fixes potential data loss on corrupt worker notifications Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>

Jet Chiang (supreme-gg-gg) force-pushed the fix/improve-postgres branch from 1d5c11f to f547a2f Compare August 18, 2026 01:29

Tim Hockin (thockin) reviewed Aug 18, 2026

View reviewed changes

Joe Betz (jpbetz) reviewed Aug 18, 2026

View reviewed changes

Jet Chiang (supreme-gg-gg) mentioned this pull request Aug 18, 2026

Move PostgreSQL expired-lease cleanup off the lock acquisition path #1040

Open

Uh oh!

Conversation

Jet Chiang (supreme-gg-gg) commented Aug 17, 2026

Uh oh!

Tim Hockin (thockin) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joe Betz (jpbetz) Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joe Betz (jpbetz) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Joe Betz (jpbetz) Aug 18, 2026 •

edited

Loading