Skip to content

fix(training-agent): allow retired GCS account replacement - #8173

Merged
bokelley merged 3 commits into
mainfrom
bokelley/training-gcs-replacement-account
Oct 14, 2026
Merged

bokelley merged 3 commits into
mainfrom
bokelley/training-gcs-replacement-account

Conversation

@bokelley

@bokelley bokelley commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

A failed private GCS canary leaves an immutable account binding. The unique organization constraint then prevents a replacement even after its destination is revoked. Permit a fresh account only when every prior account has an exact saved grant with durable revocation evidence and no current authorization.

Keep one active or unfinished account per organization. Application reservations serialize by principal and preserve immutable source facts. Migration 623 installs the database reservation guard before removing the old unique constraint, so older runtimes using INSERT ... ON CONFLICT DO NOTHING retain the same fence during rolling deployment. Retired bindings, grants, and authorization tombstones remain intact; retired destinations cannot be reused.

Validation: 21 focused checks pass, including nine real PostgreSQL controls for concurrent current and older-runtime inserts, unfinished setup, exact-generation fencing, newer live authorizations, original history preservation, immutable facts, and rollback, plus 12 existing API authorization controls. Restoring the original unique constraint reproduces the replacement failure. Normal mandatory commit hooks passed: 12,579 server tests, 1,113 root tests, guards, and both typechecks. Two HTTP teardown test files timed out during the parallel run and passed the hook’s automatic isolated rerun. The authoritative storyboard matrix runs in CI.

Integration: includes current main and the published 3.2.4 release records from merged #8153. The account reservation migration is now 623, preserving main’s independent migration 622 for compliance heartbeat ownership. The fresh PostgreSQL reservation/API controls and migration replay pass after integration; the published compliance manifest is current. A new live canary still requires explicit approval for a fresh time-bound access grant after the repairs are deployed; the original failed attempt remains unqualified.


Open workspace in Conductor

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — clean migration + application-layer defense-in-depth for training GCS account replacement.

Checked:

  • No Critical/High/Medium findings reported.
  • Migration 622 ordering installs the BEFORE INSERT trigger before dropping the old host_accounts UNIQUE constraint, so there is no unguarded rollout window (migrations run on startup in prod — judged as production-facing).
  • Idempotency, per-principal advisory locking with bounded lock/statement timeouts, and fail-closed client-release on rollback failure all verified.
  • Exact-generation retirement proof matches the always-generation-1 authorize/grant flow.
  • No changeset required: operational/training-agent code, not published protocol surface (static/schemas/source/**, reference docs, dist artifacts untouched).

High-risk flag is true only due to a newly-added migration file (server/src/db/migrations/** — (added)). Per the arbiter rules, a new file matching a high-risk glob is normal scaffolding and not escalation-worthy on the flag alone; no (deleted)/(modified) sensitive-path finding exists. gated_paths is false, no no-auto-approve team match, no prior decision. Rows 1–8 do not fire → row 9 approve.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 14, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — server-only canary-account migration with no blocking findings.

This PR adds migration 622 (BEFORE INSERT trigger on host_accounts) and reworks reserveTrainingGcsAccount() to enforce one active/unfinished training-GCS canary account per principal via a shared advisory-lock key, with retired-binding tombstones and generation-based retirement proof.

Checks confirmed by the reviewer:

  • Dropping the principal_id unique constraint breaks no read path (reads key on account_id PK / join by account_id+destination_ref).
  • ON CONFLICT (account_id) remains valid post-migration.
  • Explicit lock/statement timeouts on both paths; no hang exposure.
  • Errors rethrown (no warn-only swallowing); correct pool-poison release on rollback-failure.
  • Integration tests run against real Postgres with a localhost/adcp_test guard, covering the fence (23514), serialization, exact repair, exact-generation revocation, immutable-fact takeover, tombstone-byte preservation, and rollback cleanliness.

Not a protocol-surface change (no static/schemas/source/**, docs, or dist artifacts touched), so no changeset is required. high_risk is true only because the migration is a newly added file matching server/src/db/migrations/** — a new-file scaffolding signal, not a destructive change; it does not warrant escalation on the flag alone. No critical/high/medium findings. gated_paths is false and review_decision is APPROVED. Rows 1–8 do not fire; outcome is row 9 approve. Consistent with the prior clean pass.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — clean rebase, no blocking findings.

This PR is a pure rebase of the account-reservation migration: renamed from slot 622 to 623 to avoid colliding with main's new 622_compliance_heartbeat_claim_ownership.sql, with a corresponding bump to its RAISE EXCEPTION message. The reviewer confirmed no stale 622 references remain and that migration ordering between the two files is independent. The prior pass on head 720c6a9 was a clean approve with no findings, and nothing re-surfaces here.

Checked:

  • Migration rename (622→623) is mechanical; no logic change beyond the exception message text matching the new slot.
  • high_risk is true only because the new migration file matches server/src/db/migrations/**, but the reason is (added) — a new file is inherently low-risk scaffolding, not an escalation trigger. No (deleted) or (modified) sensitive-path concern.
  • No protocol surface touched (static/schemas/source/**, docs/reference/**, dist artifacts untouched), so no changeset/spec-drift/oneOf obligations apply. Migration-only + accompanying integration test.
  • No no-auto-approve team match; gated_paths is false.

Decision table: rows 1–8 do not fire (no critical/high/medium findings, no deleted sensitive file, no medium-on-modified-high-risk, prior decision was approve, no team gate, zero medium findings). Falls through to row 9 → approve.

@bokelley
bokelley merged commit c4a973c into main Oct 14, 2026
57 checks passed
@bokelley
bokelley deleted the bokelley/training-gcs-replacement-account branch October 14, 2026 19:00
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.

1 participant