Skip to content

ateapi/controlapi: retry worker updates on version conflict in syncer - #578

Closed
Troy Chiu (troychiu) wants to merge 1 commit into
agent-substrate:mainfrom
troychiu:fix/syncer-retry-version-conflict
Closed

ateapi/controlapi: retry worker updates on version conflict in syncer#578
Troy Chiu (troychiu) wants to merge 1 commit into
agent-substrate:mainfrom
troychiu:fix/syncer-retry-version-conflict

Conversation

@troychiu

Copy link
Copy Markdown
Contributor

This PR adds retry logic with exponential backoff when WorkerPoolSyncer encounters optimistic locking conflicts (store.ErrVersionConflict) while updating worker state in the store.

  • Before: syncWorkerToStore attempted a single UpdateWorker write. If a concurrent write modified the worker in Redis, it failed with store.ErrVersionConflict, logged an error, and silently dropped the metadata update (SandboxClass, Labels).

  • Now: On store.ErrVersionConflict, WorkerPoolSyncer retries up to 5 times with exponential backoff, re-fetching the latest worker state from Redis on each attempt before re-applying changes.

  • Tests pass

  • Appropriate changes to documentation are included in the PR

slog.InfoContext(ctx, "Syncer: updating worker in store (labels changed)", slog.String("worker", pod.Namespace+"/"+pod.Name))
w.Labels = pool.GetLabels()
changed = true
// TODO: We perform an inline retry loop here using wait.ExponentialBackoff

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just make that change now? How large would it be? IIRc The ActorTemplate controller already uses requeue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes ActorTemplateController is built using controller-runtime, which automatically provides a workqueue and handles retries when Reconcile() returns an error or RequeueAfter.

The refactor appears moderate, involving approximately 80–100 lines of changes in syncer.go to implement a PodLister, workqueue lifecycle, and worker goroutine loop.

We could keep this PR focused on the small inline backoff fix to handle version conflicts immediately, and I can create a separate PR for the refactor.

What do you think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that most of this will be replaced by that change I’d vote to just close this in favor of that, or just change this PR

@troychiu Troy Chiu (troychiu) Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I can repurpose this PR. Thanks for the feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #674 instead.

@troychiu

Copy link
Copy Markdown
Contributor Author

Closed this PR since we are going for #674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants