fix(storage): harden callback cleanup boundaries - #617
Conversation
Bound cancellation retries while preserving the first failure and continuing independent cleanup actions. Retain incomplete POSIX, Windows, and publication authority owners on the primary exception so callers can retry cleanup without closing reused foreign resources. Verified with the focused atomic-directory suite on Python 3.10, 3.11, and 3.12, the local unit tier excluding two confirmed baseline failures, and the repository pre-commit hooks for the changed files.
fishmingyu
added
the
type/bug
ChatGPT Codex Connector
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d12a3eb362
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> None: | ||
| """Keep an incomplete idempotent cleanup owner reachable from ``failure``.""" | ||
|
|
||
| if owner is None or _publication_cleanup_owner_is_closed(owner): |
There was a problem hiding this comment.
When a context body is already failing and an owned descriptor or HANDLE has concurrently been closed and reused, protect_pending_owners() reaches this probe before cleanup. Both resource owners implement closed by silently marking identity-mismatched records released, so the subsequent close_all() skips the record and the primary exception receives no ownership-change diagnostic; the same reuse is correctly reported when the body succeeds and this pre-protection path is skipped. Retaining pending owners should not destructively reconcile mismatched records before cleanup can report them.
Useful? React with 👍 / 👎.
| deque( | ||
| iter(partial(_run_ordered_actions_trampoline_pass, state), True), | ||
| maxlen=0, |
There was a problem hiding this comment.
If the callback or context body leaves a tracing/profiling hook that raises StopIteration when _run_ordered_actions_trampoline_pass is entered, this callable-sentinel iterator treats that exception as normal iteration exhaustion. _run_ordered_actions() then returns without recording a primary failure or advancing the state, so a callback result can escape with every post-validation skipped; on the cleanup path, all acquired descriptors or HANDLEs can likewise remain open. The trampoline must ensure StopIteration cannot escape into the iterator protocol as its termination signal.
Useful? React with 👍 / 👎.
| ( | ||
| ( | ||
| "directory orphan post-callback ownership validation " | ||
| "also failed", | ||
| validate_after_ownership, | ||
| ), |
There was a problem hiding this comment.
When an orphan callback catches an authenticated-file failure and also changes the orphan tree, this inner wrapper runs the ownership recapture before the generic read_child wrapper checks reader._require_valid(). The ownership-drift error therefore becomes primary and the earlier suppressed authentication failure is reduced to a later note, contrary to the reader-validity-before-ownership ordering used by _run_authenticated_directory_callback and documented for callback postconditions. Add the validity check ahead of this ownership validation so the first security failure is preserved consistently.
Useful? React with 👍 / 👎.
1 participant
Summary
Harden callback post-validation and authenticated resource cleanup as an independent replacement for the stacked #592 change.
This PR is based directly on current
main. It carries no compiler, CAS, manifest-import, or downstream artifact changes from the old storage stack.Changes
Type of Change
Testing
git diff --check origin/main...HEAD: passedChecklist