atepg: Switch to Serializable isolation level for transactions by default by ahmedtd · Pull Request #3475 · agent-substrate/substrate

Taahir Ahmed (ahmedtd) · 2026-10-30T01:18:58Z

As discussed in the community meeting, and in issue #3044, we need to switch to using Serializable isolation by default before 1.0.

Summarizing the arguments so that they are available in the commit history:

It is actually quite difficult to write fully correct applications using Read Committed or Repeatable Read isolation levels. Even Google doesn't try (we set both internal and external Spanner to default to "External Consistency", which is even stronger than Serializable).
We have not explicitly audited for correctness at the default Read Committed isolation level, so it is very likely that there are currently subtle correctness bugs in our implementation that will only be revealed under high load.

Individual code paths can use downgraded consistency levels once we have audited their use of locking or other correctness strategies. Maintaining this auditing will be a heavy ongoing commitment, so we should only do it in places where it truly matters for performance (which might end up being nowhere).

The primary side-effect of upgrading from Read Committed to Serializable is that, when there are n racing transactions that touch the same rows, it is possible for all but one of them to fail with a conflict error. The losing clients must then retry their read-modify-write loops.

Most of the complication in this change comes from ResumeActor, which is covered by a test (TestResumeActor_ConcurrentOntoOneWorker) that checks that, of 128 concurrently resumed actors targeting a single worker, none fail with a conflict error. This test seems like a bad idea, it will always be possible for it to flake. And as a matter of system/production hygiene, retry loops should almost always be driven from the outermost client (the client calling the Substrate API in this case). However, I've preserved the existing behavior for now, which required adding some additional retry loops in the workflow, and cleaning up some error wrapping so that store-layer conflict errors properly reach the workflow code.

Fixes #3044

AI use: AI-assisted, I reviewed, heavily edited, tested, and debugged.

This change is not breaking. Enjoy the same great Substrate flavor you know and love, with fewer heart-stopping data-loss incidents.

Benjamin Elder (BenTheElder) · 2026-10-30T04:19:41Z

+	defer mapSerializationFailure(&err)
 	if err := precondition.Validate(); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("in precondition.Validate", err)


should fix the Errorf without format. I assume this is meant to be:

Suggested change

return nil, fmt.Errorf("in precondition.Validate", err)

return nil, fmt.Errorf("in precondition.Validate: %w", err)

Benjamin Elder (BenTheElder) · 2026-10-30T04:20:45Z

git commit --amend --no-edit -S for the DCO.

haiyanmeng · 2026-10-30T12:48:41Z

+// write. Exported methods defer it on their named error result.
+func mapSerializationFailure(err *error) {
+	if pgErrCode(*err) == "40001" {
+		*err = fmt.Errorf("%w: %v", store.ErrVersionConflict, *err)


🤖 blocking 🔴 – Only store.ErrVersionConflict is wrapped here. The %v drops the *pgconn.PgError, so the new TestMapSerializationFailure fails on the serialization row ("no longer wraps"). Use %w for both: fmt.Errorf("%w: %w", store.ErrVersionConflict, *err).

You can't wrap two errors, can you?

Huh, you can. TIL

haiyanmeng · 2026-10-30T12:48:41Z

 	if err != nil {
 		// The snapshot's node is gone, and atelet found no upload of it.
-		if ctx.Err() == nil && status.Code(err) == codes.NotFound && localElsewhere(snap, assignment.GetNodeName()) {
+		if ctx.Err() == nil && apierror.Code(err) == codes.NotFound && localElsewhere(snap, assignment.GetNodeName()) {


🤖 blocking 🔴 – apierror.Code never reads a gRPC status, so an atelet NotFound from client.Restore now maps to Internal and this branch never fires. TestResumeActor_NodeGoneWithPendingUpload fails because of it. Please keep status.Code(err) here.

haiyanmeng · 2026-10-30T12:48:41Z

-			return true, nil
-		}
-		if errors.Is(attemptErr, store.ErrVersionConflict) || errors.Is(attemptErr, errWorkerFilledUp) {
+		if apierror.Code(err) == codes.Aborted || errors.Is(err, store.ErrVersionConflict) || errors.Is(err, errWorkerFilledUp) {


🤖 blocking 🔴 – Retrying on codes.Aborted also retries terminal errors. assignWorkerAttempt returns Aborted for "actor is X and can no longer be resumed", and that should end the resume. With this change it retries 12 times and then reports a version conflict.

haiyanmeng · 2026-10-30T12:48:41Z

-	default:
-		// assignWorkerAttempt recorded this one itself.
-		return nil, nil, err
+	if wait.Interrupted(err) || apierror.Code(err) == codes.Aborted || errors.Is(err, store.ErrVersionConflict) {


🤖 blocking 🔴 – wait.Interrupted also matches context.Canceled and DeadlineExceeded. A cancelled or timed-out resume now returns Aborted wrapping only ErrVersionConflict, and the context error is lost. ctxErr is never set any more, so recordExhaustedAssignment always gets nil. A terminal Aborted is also recorded twice, once in the attempt and again here. The existing TestEnsureWorkerAssigned_Cancelled*, _ContextEndsInsideAttemptRecordsOne and _ExhaustedBackoffRecordsOneError tests fail. The removed exhausted/ctxErr/attempted handling covered these cases.

This code was crazy complex, bordering on un-understandable. I will see if I can clean up so that recordExhaustedAssignment is only done in one place.

haiyanmeng · 2026-10-30T12:48:41Z

 	Factor:   2.0,
 	Jitter:   1.0,
-	Cap:      250 * time.Millisecond,
+	//Cap:      30000 * time.Millisecond,


🤖 should-fix 🟡 – Please remove the commented-out Cap. Without a cap, 12 steps from 1ms with jitter can sleep for about 4 to 8 s. The comment above still says "reaches Cap after 5 attempts" and "about 0.5 to 1 s". finalizeRunning reuses this backoff too, so the comment should cover both uses.

A cap is unneeded. I will fix up the comments.

haiyanmeng · 2026-10-30T12:48:41Z

+		return nil, false, fmt.Errorf("while ensuring atelet is restored: %w", err)
 	}
 	var running *ateapi.Actor
+	// TODO(ahmedtd): We are failing with concurrent update conflict here.


🤖 should-fix 🟡 – This debugging note should go before merge. If finalize still conflicts, please fix it or file an issue.

haiyanmeng · 2026-10-30T12:48:42Z

+			return false, apierror.Internal("worker %s reported epoch %d, above its registered epoch %d", workerName, workerEpoch, registered)
+		}
+
+		storedActor, err = w.store.UpdateActor(ctx, actorRef, store.PreconditionFrom(latestActor), func(toUpdate *ateapi.Actor) error {


🤖 should-fix 🟡 – Each retry re-reads the actor and sets it RUNNING without checking its state. Before this change a conflict aborted the resume. Now a concurrent write that moved the actor out of RESUMING, such as a crash, gets overwritten. Please check that latestActor is still RESUMING on the expected worker before updating.

haiyanmeng · 2026-10-30T12:48:42Z

 	if err != nil {
 		if errors.Is(err, store.ErrVersionConflict) {
-			return nil, apierror.Aborted("concurrent update conflict, please retry")
+			return nil, apierror.Aborted("concurrent update conflict, please retry: in store.UpdateActor: %w", err)


🤖 should-fix 🟡 – apierror messages go to the client as-is, so %w here sends the internal error chain, including PostgreSQL error text, to API callers. The same applies to the NotFound and InvalidArgument cases below. Please keep the client message fixed, or log the cause instead.

We need to improve the api error handling for this. Right now, this wrapping is necessary for the full error to reach the RPC log interceptor's logging of the error, which is sacred (it is very difficult to operate the system if you cannot actually determine what caused an error)/

If we want to hide error details from callers, that's fine, but it needs to be done in a way that does not erase the full error context for the RPC logging interceptor.

Discussed with Julian --- post freeze we will make some changes to the apierror package so that it can separately track the full error chain and an "opaque" message that will be presented to the caller.

haiyanmeng · 2026-10-30T12:48:42Z

+		w.recordExhaustedAssignment(ctx, lastAttempt, actor, ctxErr)
+		return nil, nil, apierror.Aborted("while running retry loop for assignment: concurrent update conflict, please retry: %w", store.ErrVersionConflict)
+	} else if err != nil {
+		return nil, nil, fmt.Errorf("while running retry loop for assigment: %w", err)


🤖 nit 🟢 – Typos: "assigment" here and "ufpdating" at line 547.

haiyanmeng · 2026-10-30T13:17:41Z

I have a few concerns about necessity, timing, and cost.

Necessity

I couldn't find a concrete anomaly that Read Committed causes in atepg today. The write paths already guard their invariants:

Updates check uid and version in the WHERE clause and return ErrVersionConflict when no row matches.
Worker capacity is decided under SELECT ... FOR UPDATE.
The worker_assignments.actor_uid primary key and the foreign keys cover the links between tables.
Leases and deletes are single statements.

I didn't find any transaction that reads with a plain SELECT and then writes based on what it read. The resume and update workflows span several store calls, and each call is its own transaction, so Serializable doesn't protect those sequences. The actor lease and the version checks still do that work.

Timing

I don't think this needs to ship in v1.0. It changes no protos, CRDs, flags, or migrations.

Measuring the impact

Serializable adds aborts and predicate-lock overhead. Clients can now get ABORTED on reads as well as writes. Could you run the cold resume benchmark at the base commit and at this PR, on the same cluster? The steps are in benchmarking/automation/ateapi-cold-resume.md. The ateapi_cold_resume_pause_fake_2cpu ladder drives BindActorToWorker and finalizeRunning, which are the transactions with the most contention. It would help to see:

the capacity step
resume p50 and p99 latency at each step
the failure count
xact_rollback from pg_stat_database

In addition to ateapi_cold_resume_pause_fake_2cpu (1 replica of ate-api-server and size0 Postgres), we could measure the performance on a set up with 8 replicas of ate-api-server and size10 Postgres:

  - name: ateapi_cold_resume_pause_fake_8x8cpu_size10
    type: locust
    description: "ate-api-server cold resume on the fake data plane, 8 ate-api-server replicas at 8 CPUs, size10 Postgres: rate ladder 5000-9000 cold resumes/s"
    targetCluster: dev
    file: /app/tests/resumepaused.py,/app/shapes/ladder_shape.py
    duration: 30m
    users: 14000
    workerCount: 200
    actorCPU: "5m"
    actorMemory: "5Mi"
    runnerCpu: "16"
    runnerMemory: "7Gi"
    isolate:
      delay: 150ms
    ateArgs:
      - "--cordon-system-infra-components"
      - "--cluster-size=size10"
    controlPlane:
      apiServer:
        replicas: 8
        cpu: 8
        memory: 4Gi
        poolMaxConns: 30
      postgres:
        args:
          - track_wal_io_timing=on
    flags:
      - "--ladder-spawn-rate"
      - "14000"
      - "--ladder"
      - "14000:3m@5000,14000:3m@5500,14000:3m@6000,14000:3m@6500,14000:3m@7000,14000:3m@7500,14000:3m@8000,14000:3m@8500,14000:3m@9000"

Julian Gutierrez Oschmann (juli4n) · 2026-10-30T13:35:25Z

I agree with haiyanmeng here. At very least we need to quantify the performance impact of a change like this. We have Isolate now to benchmark and compare.

WRT timing, I would like to have time to go deeper into the correctness concerns. I feel like we should not rush this and revisit it after v1.

Taahir Ahmed (ahmedtd) · 2026-10-30T17:12:15Z

I agree with haiyanmeng here. At very least we need to quantify the performance impact of a change like this. We have Isolate now to benchmark and compare.

WRT timing, I would like to have time to go deeper into the correctness concerns. I feel like we should not rush this and revisit it after v1.

I don't believe this framing is correct. We have started by doing something wildly unsafe (assuming we can properly code against Read Committed). Benchmarking should not block switching to a safer model.

Shruti Nair (SHRUTI6991) · 2026-10-30T17:24:10Z

I couldn't find a concrete anomaly that Read Committed causes in atepg today. The write paths already guard their invariants:

The idea of this PR is current and future looking. Concurrency race conditions are very hard to debug in production as more code (most of it is AI generated) gets added in different paths. It's better to follow safe practice before rather than having an issue in production and then changing it.

In addition to that, we shouldn't be compromising correctness for performance, every single time a new feature is added. We should be delivering feature and then optimizing.

Taahir Ahmed (ahmedtd) force-pushed the substrate-serializable branch from a3308af to 4bd9d99 Compare October 30, 2026 01:20

Taahir Ahmed (ahmedtd) requested review from Benjamin Elder (BenTheElder), Julian Gutierrez Oschmann (juli4n) and Tim Hockin (thockin) October 30, 2026 01:20

Taahir Ahmed (ahmedtd) added this to the M3 milestone Oct 30, 2026

Taahir Ahmed (ahmedtd) added the prio/P0 Highest priority / required for next milestone label Oct 30, 2026

Benjamin Elder (BenTheElder) reviewed Oct 30, 2026

View reviewed changes

Taahir Ahmed (ahmedtd) force-pushed the substrate-serializable branch from 4bd9d99 to cb67583 Compare October 30, 2026 05:05

haiyanmeng added the area/api-machinery label Oct 30, 2026

haiyanmeng suggested changes Oct 30, 2026

View reviewed changes

atepg: Use serializable isolation for transactions by default

97c0470

Taahir Ahmed (ahmedtd) force-pushed the substrate-serializable branch from b91c6cd to 97c0470 Compare October 30, 2026 22:06

	return nil, fmt.Errorf("in precondition.Validate", err)
	return nil, fmt.Errorf("in precondition.Validate: %w", err)

Conversation

Taahir Ahmed (ahmedtd) commented Oct 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benjamin Elder (BenTheElder) commented Oct 30, 2026

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!

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!

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!

haiyanmeng commented Oct 30, 2026

Uh oh!

Julian Gutierrez Oschmann (juli4n) commented Oct 30, 2026

Uh oh!

Taahir Ahmed (ahmedtd) commented Oct 30, 2026

Uh oh!

Shruti Nair (SHRUTI6991) commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Shruti Nair (SHRUTI6991) commented Oct 30, 2026 •

edited

Loading