fix(windows): report why an atomic publish failed instead of blaming the repo - #1628
Merged
Conversation
…the repo #1620: every index_repository run fails, every pass succeeds, the worker exits 0, no error-level line appears anywhere, and the user is told: Pipeline failed. Check repo_path exists and contains source files. The repository is fine. The publish is failing on a filesystem permission problem, and two separate defects conspired to hide it. cbm_rename_replace discarded GetLastError() on Windows and returned a bare CBM_NOT_FOUND. Callers then log errno - see finalize.rename_failed - so the one field that should explain the failure held whatever an unrelated CRT call left there. The Win32 error is now translated to errno, with ERROR_ACCESS_DENIED the interesting case: MoveFileEx needs DELETE on the destination, which a cache file created under an empty or foreign DACL does not grant. cbm_pipeline_finalize_staged_generation returned PERSIST_FAILED on a failed sidecar removal with no log at all. That is the silent path: a publish that fails must say so. Neither change fixes the underlying ACL problem. Together they turn a report that misdirects the user at their own repository into one naming a permission failure on a specific file - which is the difference between an unactionable issue and a five-minute one. platform + pipeline suites: 268 passed, 0 failed. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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
1 participant
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.
In #1620 every
index_repositoryrun fails like this:{"status":"error","hint":"Pipeline failed. Check repo_path exists and contains source files."}The repository is fine. Every pass succeeds,
gbuf.dumpcompletes, the supervised worker exits 0, and no error-level line is emitted anywhere. The publish is failing on a filesystem permission problem, and two separate defects conspire to hide it.1.
cbm_rename_replacediscardedGetLastError()and returned a bareCBM_NOT_FOUND. Callers logerrnoafterwards (finalize.rename_failed), so on Windows the one field that should explain an atomic-publish failure held whatever an unrelated CRT call happened to leave there. The Win32 error is now translated.ERROR_ACCESS_DENIEDis the interesting case:MoveFileExneedsDELETEon the destination, which a cache file created under an empty or foreign DACL does not grant — precisely the state #1620 and #1601 describe.2.
cbm_pipeline_finalize_staged_generationreturnedPERSIST_FAILEDon a failed sidecar removal with no log at all. That is the genuinely silent path.Neither change fixes the underlying ACL problem — #1623 and the re-stamp work address that. Together they turn a report that misdirects the user at their own repository into one that names a permission failure on a specific file, which is the difference between an unactionable issue and a five-minute one.
platform+pipelinesuites: 268 passed, 0 failed.