Repository navigation
fix: generalize database conflict errors - #2780
Merged
Merged
Conversation
Signed-off-by: Eitan Yarmush <[email protected]>
EItanya
marked this pull request as ready for review
September 9, 2026 18:57
EItanya
added a commit
that referenced
this pull request
Sep 9, 2026
…2742) Rebased onto main at `214b5568`, including #2774’s atomic `RecordRuntimeRevision` operation and #2780’s generic database error categories. This PR keeps that operation atomic and acquires pair locks before revision writes to preserve GC lock ordering. Deleting or replacing a prepared AgentTemplate left a retired pair pointing at its last successful revision. GC deleted compute and then hit a RESTRICT foreign key; it also missed checkpoint references and raced with new instance references. A single failed deletion could fail unrelated pair reconciles. Run GC as a separate leader-elected manager runnable. Controller leader election defaults to enabled even with one replica because rolling updates overlap controllers; `LEADER_ELECT=false` is available for local testing. Helm relies on that default and always grants lease permissions. GC sweeps on startup and every minute, logs each failed candidate, continues collecting other revisions, and retries durable deletions without template events. Database discovery and each candidate have one-minute deadlines. Pair reconciliation only prepares runtimes and manages references. It exclusively owns KRT readiness observations, keyed by pair and tagged with their revision; changed inputs ignore stale observations, and a fresh reconciler can clean them up without remembering previous calls. When a desired digest is being deleted, clearing its observation makes KRT derive a pending pair. The existing pending-template poll retries that state outside the error retry budget. There are no private retry or observation maps, and GC has no KRT dependency or callback. Begin deletion durably before touching Substrate. Explicit row locks in the store transactions serialize pair, instance, and checkpoint acquisition with deletion: an acquisition that wins prevents collection; deletion that wins rejects new references with ErrObjectDeleting. Pair writes check both the desired and retained last-successful revision before committing. Ordinary and scheduled instance creation acquire the revision lock in their shared insertion helper. There are no reference triggers or custom SQL exceptions. Lock the revision first, then check the shared eligibility view in a separate statement so references committed during the lock wait are visible. No database lock spans a network call. Retain revision and ActorTemplate identity until runtime cleanup succeeds. Finalization atomically releases retired success pointers and deletes the row, checks the ActorTemplate UID against stale collectors, and preserves pointers when skipped. Operations touching both pairs and revisions lock pairs first to avoid a finalization/reactivation deadlock. Preserve the current UID pair's last-good revision across invalid edits, retire replaced identities, and publish Ready only after the successful revision is persisted. Queues use ten attempts with exponential backoff from one to thirty seconds. Pending preparation still polls, but a removed pair whose retirement exhausts its retries is no longer discoverable from the graph; a restart can also lose that event. Operator repair is required until the system doctor in #2768 compares persisted identities with the synced Kubernetes graph. Leader election reduces overlapping collectors, but full stale-request fencing still requires Substrate to support UID preconditions on deletion. The schema is unreleased, so the requested changes are folded into migration 1. Existing RESTRICT foreign keys remain. The store uses main's inline pgx queries and transaction helpers; no sqlc queries or generated accessors are restored. The Migration Immutability workflow rejects this baseline rewrite; no workflow bypass is included. Validation: - PostgreSQL coverage: retirement, instance/checkpoint retention, both concurrent acquisition/deletion orderings for instances, new pairs, and reactivated pairs, both finalization/reactivation and finalization/recording lock orderings, skipped finalization, restart recovery, and stale collector UID protection. - Controller coverage: startup and periodic sweeps, list/delete failures, healthy candidates behind failed or blocked cleanup, cancellation, real KRT Ready-to-pending transitions, fresh-reconciler observation replacement and cleanup, bounded backoff, preparation retries during deletion, readiness recovery, and fresh-collector recovery after failed compute deletion, a lost response, or successful compute deletion followed by failed database finalization. - Database/controller/app/env tests, database/controller/migration race checks, and full Go lint pass. PostgreSQL coverage includes preparation of every inline SQL statement against the migrated schema. Baseline Up/Down, related service/Substrate/gRPC checks, and E2E compilation also pass. - All 279 Helm tests pass, including single-replica lease RBAC. App tests cover default-enabled election and the local opt-out. - Real Substrate lifecycle E2E validates immediate instance creation after Ready, last-good invocation, checkpoint retention, eventual ActorTemplate and golden-actor cleanup after the final checkpoint is deleted with no subsequent template change, and recreation/invocation under a new template UID. Before the KRT-ownership follow-up, the strengthened test passed in 85 seconds while the preserved `runsc state` exit-128 deletion failed on each independent sweep; unrelated pair reconciles no longer reported that GC error. Live Substrate E2E was not rerun after the KRT changes or this rebase: the dedicated test cluster was removed before rollout. GC metrics remain in #2744; failed or abandoned checkpoint recovery remains in #2746. Sweeps are serial; bounded workers can be added if slow deletions cause a material backlog. --------- Signed-off-by: Eitan Yarmush <[email protected]>
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.
Database conflicts use object-specific sentinels, and clients can receive misleading reasons: checkpoint creation is reported as an active task, while lifecycle conflicts are reported as missing quiescent boundaries.
Use shared
ErrConflictandErrFailedPreconditioncategories across agent instances, tasks, checkpoints, and scheduled runs. Wrap them with the resource and blocking condition, preserve that reason in service and A2A responses, and keep idempotency conflicts distinct for their existing client handling.Validation:
make -C go lintandgit diff --check.