Repository navigation
feat(authz): recover external owner writes from durable intents - #2187
AlexStocks wants to merge 5 commits into
Conversation
Persist trusted owner and candidate attestation intents in the business transaction, then replay them outside it before returning success. Retain stable audit identity across retries, process restarts and lost acknowledgements. Expose bounded, content-free operator inventory without inferring historical owners. Constraint: Preserve immutable owners, exact candidate targets and independent read grants; retain same-database atomic writes and domain-specific security requirements. Scope-risk: Adds a Server-owned recovery table for external-store compositions. Artifact recovery requires an event-idempotent audit store; custom assemblies without commit hooks retain their existing retry guarantees. Tested: Windows ownership/Access regressions 129 passed; Linux recovery/document tests 27 passed; changed-file Ruff/format/pre-commit checks and full Linux Ty/Python 3.11 passed. SQLite first-SAVEPOINT rollback regression fails without the transaction guard.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Read pending operation IDs before decoding each intent so malformed metadata cannot starve unrelated recovery work. Preserve fail-closed explicit replay. Exercise provider disappearance with the real built-in Access service rather than an incomplete startup-only test double. Constraint: Keep original ownership and all provider-disappearance denial assertions; never repair a damaged intent by guessing an identity. Scope-risk: Changes only recovery polling and the startup test fixture. Tested: Damaged-page regression fails before the fix; focused recovery 19 passed, Windows ownership/Access regressions 130 passed, Server suite 58 passed.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
Two issues remain: an idempotent publication retry can succeed before its required audit is delivered, and the new background-recovery test can cancel delivery before asserting the audit.
| ), | ||
| **({"on_commit": on_commit} if access is not None else {}), | ||
| ) | ||
| await flush_owner_writes() |
There was a problem hiding this comment.
[P2] Finish the original recovery intent on an idempotent retry
If append_audit_once fails after the external owner commits, the first publication request returns 503. Retrying the same payload/key returns the existing publication without executing on_commit, so this session has no tracked operation IDs and flush_owner_writes() does nothing. The subsequent owner-only check passes. With two real SQLite stores, I reproduced 503 followed by 201 while the audit remained unavailable, the intent remained pending, and no artifact-owner-established audit existed. Before returning success, retries must complete the original accepted intent or retain the failure; preserve its original identity and audit context.
There was a problem hiding this comment.
Fixed in 8ccd0c7. Idempotent publication retries replay the original persisted Artifact intent before returning success, including its original audit event. The same gap existed for Handoff commit retries and is fixed there as well. No ownership is restaged from the retrying Principal or request context.
Both HTTP regressions reproduce the original success response while audit remains unavailable. They now retain 503 until the audit recovers, then complete one original audit event, clear the intent and return the same domain result on repeat. The checks use two independent real SQLite stores and verify original request correlation and no duplicate Artifact/audit.
| async with recovery.monitor(), asyncio.timeout(12): | ||
| while await access.artifact_owner(other) is None: # noqa: ASYNC110 - observe persisted external state | ||
| await asyncio.sleep(0.05) |
There was a problem hiding this comment.
[P2] Wait for delivery completion before stopping the monitor
The owner relation becomes visible before _deliver_artifact finishes writing its audit. Once this polling loop observes the relation, exiting recovery.monitor() cancels the worker. Cancellation between those writes leaves zero audit events, so the assertion below fails even though recovery correctly retains the pending intent. I reproduced this failure in the unmodified test; a subsequent run passed. Keep the monitor running until the healthy intent's delivery/audit is complete before asserting the result and cancelling the worker.
There was a problem hiding this comment.
Fixed in 8ccd0c7. The monitor stays active until the healthy Scope has no pending delivery intents, so the audit and primary acknowledgement finish before cancellation. The deliberately corrupted row remains in its separate damaged Scope and is still explicitly rejected. All original owner and audit assertions are preserved.
Replay the persisted Artifact intent when publication or Handoff returns an idempotent result without invoking its commit hook. Keep the original Principal and audit context and retain failure until delivery finishes. Wait for durable recovery completion before cancelling the monitor test. Constraint: Never create or restage ownership from the retrying identity. Scope-risk: HTTP publication/Handoff retries with external ownership and audit stores. Tested: Both new HTTP cases fail before the fix; Windows recovery/HTTP 21 passed; Linux recovery/HTTP/access regressions 86 passed; changed-file hooks and full Linux Ty/Python 3.11 passed.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
| async def flush_owner_writes(*, artifact: ResourceRef | None = None) -> None: | ||
| session = _SESSION.get() | ||
| if session is not None: | ||
| while session.operation_ids: |
There was a problem hiding this comment.
[P2] Replay pending ownership intents on Profile flush retries
After a Profile flush commits but external delivery fails, retrying can return review_pending or noop without invoking the commit hook, leaving this session's operation IDs empty and returning success while ownership delivery remains incomplete. Replay the original persisted Profile or Candidate intent before allowing these retry responses to succeed.
There was a problem hiding this comment.
Fixed in e8c79f0. HTTP and scheduled Profile flush responses now replay the original persisted Profile intent and the returned Candidate intent before returning success, including noop/review_pending retries that bypass the commit hook. No owner or audit context is restaged from the retry identity.
Real HTTP regressions with two independent SQLite stores reproduce the previous 200 response while relationship or audit delivery remains unavailable. Retries now retain 503 and the same pending intent until delivery recovers, then preserve the original owner/request correlation and return the existing Profile or Candidate without duplicate domain writes.
Complete the original persisted Profile or Candidate ownership intent before HTTP and scheduled flush retries return noop or review_pending. Keep the accepted identity and audit context when external delivery is still failing. Constraint: replay only existing durable intents; never stage ownership from retry identity or infer ownership for historical records. Scope-risk: external ownership delivery on Profile flush response paths. Tested: focused recovery, authorization and Profile HTTP suites (95 passed); HTTP/background snapshot authorization (2 passed); full Python 3.11 ty check; scoped prek hooks. The three new HTTP cases failed before the source fix. Refs: oceanbase#2084, oceanbase#2187
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
hidb4ai
added
1.5.0
Merge current master into external owner recovery. Keep relational batch attestation and stage external batch intents in the original domain transaction. Remove the unused cast import exposed by the automatic merge. Constraint: Preserve original proposer, exact target, rollback and replay semantics. Scope-risk: The scheduled Candidate ownership hook now supports both batch attestation and durable external delivery; no changes to another author's branch. Tested: Focused recovery, HTTP Access, Profile, scheduler and Server tests (173 passed, 2 disabled native backend cases skipped); batch regressions (2 failures before implementing batch staging); prek run -a with ty-check run separately; Linux Ty/Python 3.11 with the standalone acceptance-fixture search path (all checks passed); git diff --check.
|
Resolved the master conflict in The scheduled Candidate hook retains master's batched relational attestation and this PR's durable external ownership intents. Batch commit/rollback and reopen/replay regressions preserve the original proposers and exact targets. Focused checks: 173 passed, 2 disabled native backend cases skipped. Lint/format hooks and full Linux Ty with the documented fixture search path passed; the PR body records the environment limits and exact command. New-Head CI and reviewer confirmation remain pending. |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Which issue or RFC does this PR close?
Closes #2159. Refs #2084 (R4), RFC 1396 and merged #2164.
Rationale for this change
When ownership and audit live outside the business transaction, a committed Artifact or Candidate can outlive a failed owner write. Retrying as the current caller must not replace the original initiating identity. Historical catalogs may also have no trusted ownership evidence at all.
What changes are included in this PR?
pc_access_owner_recoveryalongside supported domain writes. Retain only trusted Principal identity, exact resource/Candidate/target, original idempotency key and minimized audit context.IdempotentAccessAuditStore.append_audit_onceand its canonical relational implementation. Stable recovery event IDs prevent duplicate audit events after lost acknowledgements. A missing external audit capability rolls the domain write back before acceptance.pending, metadata-only paginatedinventory_gaps, andreplay(operation_id). Unknown historical owners are reported without reading protected bodies or assigning the current user. Completed receipts are retained.Are there any user-facing changes?
External-store creation paths can now recover after restart using the original trusted identity. Delivery failure still returns 503; a non-idempotent create must not be repeated blindly. External Artifact recovery requires event-idempotent audit. No owner-assignment endpoint, HTTP/MCP recovery API, CLI, ownership transfer or automatic historical backfill is added.
An independent read grant remains valid for an ownerless identity; reading never establishes ownership. Legacy Memory collection writes remain unsupported. Atomic Memory, Dream and supervised-worker security keep their existing domain transaction requirements.
How was this change tested?
Current source validation for
aa2919265028eea1eef758ec3ce2b1b0d4d516e2, including master6b976859195b6310960f7d7f9ea5bce423ebaaa8:pytest tests/e2e/test_scheduled_experience_incubation.py tests/test_owner_recovery.py tests/e2e/test_owner_recovery_http.py tests/e2e/test_access_control_regressions.py tests/e2e/test_profile_api.py tests/test_access_http.py tests/test_server.py -q --tb=short: 173 passed, 2 skipped. The two skips are the opt-in seekDB/OceanBase batch-incubation cases; the SQLite case and scheduled processing test passed.SKIP=ty-check prek run -a; Ty was run separately with the Linux environment:ty check --python /tmp/pc2167-venv --python-version 3.11 --extra-search-path tests/e2e/cross_host_work_continuity/fixturespassed. The explicit search path resolves the existing standalone acceptance script'sfrom task import increment; no repository type-check configuration changed. The default WSL invocation cannot resolve the Windows virtualenv. Plain full Linux Ty without that fixture search path reports the pre-existing standalone import; a defaultmake checkpass is not claimed.git diff --checkpassed. The only manual merge conflict was the scheduled Candidate owner hook: same-database batch attestation is retained, while external batches stage durable intents before domain commit.Limits: these results establish separate SQLite store behavior and application lifecycle recovery. Remote relationship-service, OceanBase/MySQL and multi-Server ownership recovery acceptance were not executed. Existing custom assemblies without domain commit hooks do not gain this guarantee. New-Head GitHub CI and independent review remain required.
AI usage statement
Implemented and verified with OpenAI Codex. AI-generated changes were checked against the current contracts, existing Access regressions and real SQLite failure-injection tests.