Repository navigation
mingw: let reftable retry reads of tables pending deletion - #143
Open
friel-openai wants to merge 2 commits into
Open
friel-openai wants to merge 2 commits into
friel-openai wants to merge 2 commits into
Conversation
friel-openai
added
git
friel-openai
marked this pull request as ready for review
October 14, 2026 20:29
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
CreateFileW() returns ERROR_ACCESS_DENIED when a file is pending deletion. With legacy deletion, another open descriptor can keep a deleted reftable in this state. A reader that obtained tables.list before compaction then fails with EACCES instead of rereading the updated list. Map STATUS_DELETE_PENDING to ENOENT when opening an existing file. Keep other access-denied errors unchanged, as well as the existing EEXIST mapping for O_CREAT | O_EXCL. Resolve RtlGetLastNtStatus before CreateFileW so loading the function cannot overwrite the status being inspected. Add a regression that deletes a file while retaining an open descriptor and checks that reftable reports REFTABLE_NOT_EXIST_ERROR. Exercise both default and legacy deletion without timing-dependent concurrent processes. Signed-off-by: Friel <[email protected]>
Starting the topic from master restores workflows with unpinned action references. Retain the exact workflow blobs accepted by ci_workflow_pins_are_reviewed() so the source branch keeps the CI configuration used to validate the fix. Signed-off-by: Friel <[email protected]>
friel-openai
force-pushed
the
af/codex/reftable-delete-pending
branch
from
October 16, 2026 05:23
caf74e2 to
b1013cd
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Windows, a reftable deleted by compaction can remain in
STATUS_DELETE_PENDINGwhile a handle is open.CreateFileW()reportsERROR_ACCESS_DENIED, which becomesEACCESinmingw_open_existing(). Reftable then aborts the read instead of rereadingtables.listafter compaction.Report
ENOENTwhenCreateFileW()fails with the specific native statusSTATUS_DELETE_PENDING. This lets reftable's existing missing-table retry work. Other permission errors and the existingO_CREAT | O_EXCLhandling are unchanged. Add deterministic tests that retain a descriptor across deletion and requireREFTABLE_NOT_EXIST_ERROR, using both default and legacy Windows deletion.The source branch starts from verified public
master(b8242b093d9e), rather than a generatedcodextip. The fix and regression patch is unchanged by this ancestry correction (git range-diffreports equality). A separate commit retains the three exact CI workflow blobs accepted by the controller. The review target remainscodex; this is a review-only topic PR.The investigation started with t0610.48 failing to resolve HEAD while validating #141. That PR changes send-pack/fetch-pack negotiation and unrelated tests; it does not change reftable, update-ref, or Windows file operations. The failing command performs a local reference update and does not call the changed negotiation code.
Before the fix, deterministic Windows tests reproduced
STATUS_DELETE_PENDING,EACCES, and a failed reftable reload during compaction. With the fix, the permanent default/legacy deletion tests pass. A separate temporary test also confirmed that POSIX deletion exposes the same native status before the deleting handle closes, and that the corrected open reportsENOENT. The original failing job did not record its native error, so its connection to this bug remains an inference. Temporary diagnostics and compaction callbacks are excluded from this PR.The same base tree passed Windows CI on October 5. Its concurrent-writer test did not deterministically retain a deleted table in the pending state.
mingw_open_existing()and its access-denied handling originated in a270cb1540, authored by Patrick Steinhardt. The new regression covers that missing state without relying on scheduling.Validation: Windows Make and Meson CI both passed
t0610on the previous source, including the permanent default/legacy deletion regressions. The corrected source also passes Windows Maket0610, the Linux DEVELOPER build, all 93 localt0610tests, 29 reftable stack unit tests, andgit diff --check. Standalone topic CI remains prerequisite-dependent; the combined validation below is green.Standalone source CI exposes existing HTTP failures that this Windows-only change does not affect: WebDAV and dumb-HTTP authentication, covered by #142 and its #126 prerequisite, and Debian 12 HTTP/2 authentication. The latter is already quarantined for affected curl versions by 003e6089a2, which is present in generated
codexbut absent from this topic'smasterancestry. The same HTTP/2 failure reproduces on #141's unchanged prerequisite. These failures are not evidence of a reftable regression, and the standalone matrix is not fully green.A CI-only combined candidate tests #141, #142, this PR, and their declared HTTP prerequisites, including the existing Debian quarantine and its gzip-authentication prerequisite. All 306 local tests across
t0610,t5540,t5549,t5550,t5556, andt5702pass on that composition with curl 8.23. Combined CI completed successfully: 45 jobs passed, two expected jobs were skipped, and none failed. The candidate leaves topic source ancestry intact and contains no release or controller workflow; it does not publish a Git release.This changes client error handling only. Repository formats and the compaction protocol are unchanged, so clients can be updated independently. Reverting restores the read failure for pending deletion.
The tested candidate is
2006a35b7b78c1024e56f1658f7f1190f4b76a7c, assembled from public masterb8242b093d9e941a34460d715e3ce616a34ac3fewith these exact sources: explicit-haves prerequisitea3eae730133768ea334731d2f48a80aff724fb5d, #141772653bcb2b069505961b0b10604b0abf5a13062, HTTP redirect prerequisiteb81ca645fdf0bf70789691823f9d7a4abbb05697, gzip authentication prerequisite50f753ee38ef84de2fc22ca5cdde6698f0479f85, Debian HTTP/2 quarantine003e6089a2598e2f37eb2326ab3947836ebb86c0, #1420ad3526206bbe3c820e9aced7832c6d3e2c9e7ac, and #143b1013cd9c374194c94a35889dca67d7ca57fdb7a. This validates the combined source; human review and normal controller staging remain required.-frielbot
Codex Task:
codex://threads/01a13b95-2df7-75a1-bc99-4266c08d7d89