fix(sdk): close() must attempt every as_path file - #352
Merged
domenkozar merged 1 commit intoAug 16, 2026
Conversation
dngr2
force-pushed
the
fix/sdk-close-attempts-every-as-path-file
branch
from
August 16, 2026 02:46
88f515e to
d1700c6
Compare
Member
|
@copilot please fix the merge conflicts in this pull request. |
Resolved.close() exists so secret-bearing temp files do not outlive the result. The Python and Ruby SDKs stopped at the first file the OS refused to remove, so every later secret stayed on disk — the outcome close() is there to prevent — and the caller had no way to learn which ones survived. Demonstrated with three as_path secrets where the second cannot be removed: two of the three files remained, contents intact. The Go and .NET SDKs already do this correctly, recording the first failure and attempting the rest (secretspec.go:152 firstErr, Models.cs:90 firstError). .NET catches IOException specifically, the ordinary Windows sharing violation raised when another process still holds the file open, so a failed delete is already treated as an expected condition elsewhere in the project. Both SDKs here now follow that same contract: clean up everything, then raise the first error. Ruby additionally no longer skips a dangling symlink. The guard was File.delete(path) if File.exist?(path), and File.exist? follows symlinks, so a broken link reported absent and was never removed; rescuing Errno::ENOENT instead also closes the check-then-delete race. Tests cover the ordinary path, idempotency, that a refusal does not strand the other files, and that the first error is the one raised. Reverting either fix fails the stranding test specifically.
dngr2
force-pushed
the
fix/sdk-close-attempts-every-as-path-file
branch
from
August 16, 2026 14:03
d1700c6 to
04e6bc4
Compare
Contributor
Author
|
Rebased onto main. The only conflict was in CHANGELOG.md, between this entry and the one from #358; both are kept. Re-ran both SDK suites on the new base: Python 19 passed, Ruby 18 runs across three files. |
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.
The bug
Resolved.close()exists so secret-bearing temp files do not outlive the result. The Python and Ruby SDKs stop at the first file the OS refuses to remove, so every later secret stays on disk — the one outcomeclose()is there to prevent — and the caller has no way to learn which ones survived.Demonstrated with three
as_pathsecrets where the second cannot be removed:Why this is a contract bug, not a preference
The Go and .NET SDKs already handle it — they record the first failure and attempt the rest:
secretspec.go:152firstErr, every file attemptedModels.cs:90firstError, throws after the loopResolved.php:72@unlink, loop continues__init__.py:142secretspec.rb:90.NET catches
IOExceptionspecifically — the ordinary Windows sharing violation raised when another process still holds the file open. So a failed delete is already treated as an expected condition in this project; Python and Ruby simply do not implement that half of the contract. Both now follow it: clean up everything, then raise the first error.A second Ruby bug this closes
The Ruby guard was
File.delete(path) if File.exist?(path).File.exist?follows symlinks, so a dangling symlink reports absent and is never removed — it just accumulates in the temp dir. RescuingErrno::ENOENTinstead removes that case and the check-then-delete race together. Called out separately in the changelog rather than folded into the main entry.Verification
Both SDKs were built and tested against the real compiled extensions, not stubs.
maturin develop --uv; 5 new tests pass, existing suite 19 passscripts/build-ext.sh; 4 new tests pass, existingtest_resolve.rb14 runs / 34 assertions / 0 failuresMutation-tested: reverting either fix fails
test_close_removes_the_rest_when_one_file_cannot_be_removedon exactly the intended assertion — "file after the failure was stranded on disk". The other tests still pass under that mutation, so the stranding case is the one carrying the regression.Tests cover the ordinary path, idempotency, that a refusal does not strand the other files, and that the first error is the one raised.
One note in the interest of full disclosure:
secretspec-py/tests/test_codegen.py::test_quicktype_types_consume_runtime_fieldsfails in my environment withERR_REQUIRE_ESMfromquicktypeunder Node 18. I confirmed it fails identically with my changes stashed, so it is pre-existing and environmental, but I could not get that one test green locally.CHANGELOG entry added under the existing
### Fixedheading in[Unreleased], perAGENTS.md.