Skip to content

fix: generalize database conflict errors - #2780

Merged
EItanya merged 1 commit into
mainfrom
fix/generic-database-errors
Sep 9, 2026
Merged

EItanya merged 1 commit into
mainfrom
fix/generic-database-errors

Conversation

@EItanya

@EItanya EItanya commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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 ErrConflict and ErrFailedPrecondition categories 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:

  • PostgreSQL database tests and affected service, A2A gateway, gRPC, and scheduled-run controller tests.
  • Regression coverage for wrapped conflict classification and client-visible reasons.
  • make -C go lint and git diff --check.

@github-actions github-actions Bot added the bug Something isn't working label Sep 9, 2026
@EItanya
EItanya marked this pull request as ready for review September 9, 2026 18:57
@EItanya
EItanya requested a review from a team as a code owner September 9, 2026 18:57
@EItanya
EItanya merged commit 214b556 into main Sep 9, 2026
28 checks passed
@EItanya
EItanya deleted the fix/generic-database-errors branch 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant