Skip to content

fix(credential-proxy): settle companion-revision policy withdrawals - #3283

Merged
freeqaz merged 4 commits into
mainfrom
fix-2235
Oct 23, 2026
Merged

freeqaz merged 4 commits into
mainfrom
fix-2235

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

With a Credential Proxy Driver that declares policyReduction (no bundled Driver does; the protected-execution fixture does), withdrawing a protected source from an Agent also records withdrawals for its companion revisions (credentialWithdrawalCompanionRevisions): unretired predecessors and every later admitted revision, for example a candidate whose deployment failed or is still running.

reduceCredentialPolicy only reduces the policy of the Agent's active revision. For any other revision it threw a plain Error ("no exact admitted owner or withdrawal"). The Kubernetes Compute Driver passed it through, the worker classified it as retryable DEPENDENCY_UNAVAILABLE (not a replay reason), and the companion's withdrawal was retried on every maintenance pass (or, without maintenance, through the hour-long recovery chain) without ever settling. While an attempt was queued, deleting the source answered CREDENTIAL_WITHDRAWAL_IN_PROGRESS. Egress stayed refused throughout, because any withdrawal row blocks use on that revision, so this was a liveness bug, not an exposure.

Change

reduceCredentialPolicy now returns retire-runtime when a source withdrawal is recorded for the revision and another revision is the Agent's active one. The check runs inside the existing transaction, under the Namespace and Agent locks, before any reduction is recorded or dispatched. Compute then reports the existing CREDENTIAL_WITHDRAWAL_REQUIRES_RUNTIME_RETIREMENT refusal. The worker records it once as a permanent result that waits for a replay, and maintenance no longer re-queues it.

Why this direction rather than treating "withdrawal recorded and no installation ever recorded" as withdrawn:

  • A still-deploying candidate can record its installation after that check. Marking its withdrawal done would let a runtime hold the credential with a settled withdrawal. retire-runtime never marks anything withdrawn.
  • The withdrawal row stays pending, so OCC keeps refusing egress for the source on that revision (checkEgressUseIn and the send authorization both refuse on any withdrawal row).
  • Nothing is dispatched to the peer and no reduction operation is recorded, so replay protection and the active-revision path are unchanged.

Unchanged on purpose:

  • A call without a recorded withdrawal still fails as before (it is never turned into a retirement request).
  • An Agent with no active revision still fails as before (the existing "loses active ownership" case).
  • Repository Git cleanup (sourceId undefined) is unaffected: it never has a source withdrawal row.

How it settles: the next deployment that succeeds retires every earlier version, including the companion, which also drops it from the read's companion walk. If the companion becomes the selected version instead, a replay reduces it in place. The read can report the companion's pending withdrawal (with its revisionId and withdrawalInProgress: false, which already means "replay needed"). The withdrawal reference documents the reason code, the lifecycle flow traces the refusal, and the two operator guides now send a reader whose revisionId names another version to that reference instead of the stop procedure.

The test fixture now records deployment work (recordOperations: true through the shared postgresApp helper), so it can tell a failed candidate from one still deploying.

Verification

  • tests/integration/credential-proxy-withdrawal.test.ts (PostgreSQL Platform lane) now deploys two later candidates: one whose reconcile work fails permanently through the real work queue, then one still queued. The real withdraw route records a companion row for each. Through the real Kubernetes Compute Driver, each candidate's withdrawal now rejects with CREDENTIAL_WITHDRAWAL_REQUIRES_RUNTIME_RETIREMENT; its row stays pending; it records no reduction operation; and the peer receives no reduction (applies stays 0). Before the withdrawal, the same call on a candidate still fails with the original "exact admitted owner" error.
  • The same file covers a revision that loses active ownership to the deploying candidate while its reduction is recorded but unobserved: it now requires retirement, the peer gets no second apply, and the recorded operation stays unobserved. Restoring active ownership then settles it as before.
  • The new assertions fail on main (Comparison {} instead of the expected code) and pass with the fix.
  • Mutants on the fix, all killed by this file: dropping the new branch; dropping the "an active revision exists" check; dropping the recorded-withdrawal requirement; dropping the "not the active revision" check; returning pending instead of retire-runtime; applying the branch only without a recorded reduction; dropping the transaction result passthrough.
  • 20/20 sequential runs at 2 CPUs (taskset -c 0-1), 3.2 s to 3.8 s each, against one prepared lane database reused across runs.
  • ClawSweeper's review of 1a53e15a3 asked for the lifecycle flow update; it is in 6fc32e0d8.
  • Internal review (security-focused, high effort): approve with nits. Applied: the custody case above, earlier placement of the check, clearer test comments, and the guide and reference wording. Not applied: re-queueing a candidate's own refused withdrawal when it activates (see Risks); an unretired-predecessor case, which runs the same branch and needs a protected predecessor this fixture does not build.
  • The worker's handling of this refusal (permanent, waits for a replay, other sources in the batch still withdrawn) is already covered by postgres-worker-agent-revision-credential-sources.test.mjs.
  • tsc --build tsconfig.json tests/tsconfig.json, ESLint and Prettier on touched files, pnpm docs:check and the flow-doc validator pass. The lifecycle flow is at 2497 of 2500 words.

Risks

  • A predecessor that is still serving while its successor deploys now ends its withdrawal with the refusal at once, instead of retrying until the successor retires it. It retires the same way; its row just keeps the refusal reason. Egress for the source stays refused on it either way.
  • A still-deploying candidate that later becomes selected needs a replay to reduce its policy in place. Before this change it could settle on its own if it activated while retries were still running. The read reports it as a pending withdrawal with no attempt in progress, and its egress for the source stays refused until the replay. Queueing one attempt when a candidate activates would remove the manual step; that is left for a follow-up.
  • No bundled Driver declares policyReduction, so production installs with bundled Drivers see no change.

A protected-source withdrawal also records rows for companion revisions
(unretired predecessors and later candidates). reduceCredentialPolicy only
reduces the active revision's policy, and for any other revision it threw a
plain error, which the worker retried as DEPENDENCY_UNAVAILABLE and maintenance
re-queued forever. Return retire-runtime for a recorded withdrawal on a
revision that is not the Agent's active one, so Compute reports the existing
replay-awaiting refusal. The row stays pending, so egress stays refused.
Record deployment work in the protected withdrawal fixture so one later
candidate can fail for good while another stays queued. Both get companion
withdrawal rows from the real withdraw route and must report
CREDENTIAL_WITHDRAWAL_REQUIRES_RUNTIME_RETIREMENT without dispatching a
reduction; a call without a recorded withdrawal still fails as before.
Document the reason in the withdrawal reference.
@freeqaz
freeqaz requested a review from a team as a code owner October 23, 2026 07:41
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 23, 2026
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR replaces repeated companion-revision withdrawal failures with the existing retirement refusal and documents replay recovery.

Example: Version 2 is selected while versions 3 and 4 hold the same protected credential source, with one deployment failed and the other queued.

  • Before: Withdrawing the source makes both companion reductions throw an error that triggers repeated retries.
  • After: Both return CREDENTIAL_WITHDRAWAL_REQUIRES_RUNTIME_RETIREMENT without dispatching a reduction, leaving their withdrawals pending for retirement or replay.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported repair with meaningful integration coverage and the previous finding resolved; supplied behavior evidence remains harness-based.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored PR is outside the ordinary contributor proof gate; its supplied evidence covers the reducer through a PostgreSQL integration harness, including companion refusal and preserved unobserved custody, without claiming released OpenShell enforcement.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Withdrawals on failed or deploying companion revisions repeatedly retry a condition that cannot settle through in-place policy reduction.
Reason: The maintainer-authored direction repairs a concrete liveness failure while explicitly accepting and documenting replay when a companion later becomes selected.

Merge readiness

✅ Ready for maintainer review

The previous flow-documentation finding is resolved, and no new actionable defect was found; this maintainer-authored PR remains open for landing review.

Priority: P2
Reviewed head: 6fc32e0d846453e85ff56ca9c351106627ea7988

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The supplied verification uses PostgreSQL, Fastify injection, real Compute and reducer code, and a controlled peer rather than a deployed Driver workflow.
Agent review details

How this fits together

OCC records authorized source withdrawals in PostgreSQL; the worker invokes Kubernetes Compute and the policy reducer, then records withdrawal results and recovery work.

flowchart TD
  A[Authorized withdrawal request] --> B[OCC withdrawal rows]
  B --> C[Controller worker]
  C --> D[Kubernetes Compute]
  D --> E[Policy reducer]
  E -->|Selected revision| F[Credential Proxy peer]
  E -->|Companion revision| G[Retirement refusal]
  G --> C
Loading

Technical review

Best possible solution:

Use the existing retirement-refusal path for companion revisions while preserving pending withdrawals and documented replay recovery.

Do we have a high-confidence way to reproduce the issue?

Source inspection confirms that companion withdrawals reach main's selected-revision error and the worker's retry classification; the author reports base-failing and head-passing PostgreSQL runs, which were not rerun during this read-only review.

Is this the best way to solve the issue?

The change is a focused repair in the existing reducer, preserves its selected-revision invariant, and reuses Compute and worker handling rather than adding another recovery mechanism.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 6fa1aa00c9f9.

Provenance checked

  • Selected-revision policy reduction changes intended behavior with a stated reason (3115679: The Credential Proxy implementation requires an exact admitted owner and selected revision before recording or dispatching policy reduction.)
  • Withdrawal refusal and replay lifecycle keeps the original intent (2f38b71: Refused withdrawals remain pending and wait for authorized replay instead of repeating failures through maintenance.)
  • Operator retirement guidance changes intended behavior with a stated reason (3115679: An execution whose reduced policy cannot be verified requires observed retirement before an ownership or delivery cutover.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • freeqaz: Raw commit 2f38b71 adds docs/flows/credential-source-lifecycle.md:244 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 2f38b71ed038; files: docs/flows/credential-source-lifecycle.md)

Labels

Label changes:

  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This repairs repeated withdrawal work for Drivers advertising policy reduction; no bundled Driver advertises it, limiting current user impact.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-23T07:54:56.054Z sha 1a53e15 :: needs changes before merge. :: [P3] Update the owning withdrawal flow for the new refusal branch

Reviewed October 23, 2026, 4:01 AM ET / 08:01 UTC (Revision 2).

Check for another active revision right after the withdrawal lookup, before
the repository-attempt read. Cover a revision that loses active ownership to
the deploying candidate while its reduction is recorded but unobserved: it
requires retirement without observing or repeating the operation. Explain the
claim loop and the recorded deployment work in the test. Point the operator
guides to the withdrawal reference when revisionId names another version.
… lifecycle flow

reduceCredentialPolicy reduces protected sources only on the selected
version; elsewhere it sends nothing and the refusal awaits a replay. List
CREDENTIAL_WITHDRAWAL_REQUIRES_RUNTIME_RETIREMENT among the codes that fail
withdrawal work at once.
@freeqaz

freeqaz commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

Added the lifecycle flow update (section 9 and the reason-code list) in 6fc32e0, plus a superseded-revision custody case in the test and guide wording from an internal review.

@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 23, 2026
@freeqaz
freeqaz merged commit f2a5b4b into main Oct 23, 2026
64 of 67 checks passed
@freeqaz
freeqaz deleted the fix-2235 branch October 23, 2026 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant