Skip to content

fix(sdk): close() must attempt every as_path file - #352

Merged
domenkozar merged 1 commit into
cachix:mainfrom
dngr2:fix/sdk-close-attempts-every-as-path-file
Aug 16, 2026
Merged

fix(sdk): close() must attempt every as_path file#352
domenkozar merged 1 commit into
cachix:mainfrom
dngr2:fix/sdk-close-attempts-every-as-path-file

Conversation

@dngr2

@dngr2 dngr2 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

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 outcome close() is there to prevent — and the caller has no way to learn which ones survived.

Demonstrated with three as_path secrets where the second cannot be removed:

before close:  [True, True, True]
close() raised PermissionError: [Errno 13] Permission denied: '/tmp/secret1_kxwezsvs'
after close:   [False, True, True]

secret files still on disk: 2 of 3
   /tmp/secret1_kxwezsvs  contents='super-secret-value'
   /tmp/secret2_0_99cy2z  contents='super-secret-value'

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:

SDK Behaviour
Go secretspec.go:152 firstErr, every file attempted
.NET Models.cs:90 firstError, throws after the loop
PHP Resolved.php:72 @unlink, loop continues
Python __init__.py:142 aborts, strands the rest
Ruby secretspec.rb:90 aborts, strands the rest

.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 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. Rescuing Errno::ENOENT instead 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.

  • Pythonmaturin develop --uv; 5 new tests pass, existing suite 19 pass
  • Ruby — native extension built via scripts/build-ext.sh; 4 new tests pass, existing test_resolve.rb 14 runs / 34 assertions / 0 failures

Mutation-tested: reverting either fix fails test_close_removes_the_rest_when_one_file_cannot_be_removed on 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_fields fails in my environment with ERR_REQUIRE_ESM from quicktype under 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 ### Fixed heading in [Unreleased], per AGENTS.md.

@dngr2 dngr2 reopened this Aug 15, 2026
@dngr2
dngr2 force-pushed the fix/sdk-close-attempts-every-as-path-file branch from 88f515e to d1700c6 Compare August 16, 2026 02:46
@domenkozar

Copy link
Copy Markdown
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
dngr2 force-pushed the fix/sdk-close-attempts-every-as-path-file branch from d1700c6 to 04e6bc4 Compare August 16, 2026 14:03
@dngr2

dngr2 commented Aug 16, 2026

Copy link
Copy Markdown
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. test_codegen fails in my environment on main as well, from a quicktype/stream-json ESM mismatch with the local Node, so it is unrelated.

@domenkozar
domenkozar merged commit 3f5a376 into cachix:main Aug 16, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants