fix(sdk): close() must attempt every as_path file

CHANGELOG.md

@@ -66,6 +66,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   spelled `n-u-l-l`. The `bw` and `dashlane` providers already behaved this way.
   An `extract` pointer is unchanged: it names one location and still reports a
   `null` there, and the two policies now sit next to each other in one place.
+- The Python and Ruby SDKs' `Resolved.close()`/`Resolved#close` now remove every
+  `as_path` temp file even when one of them cannot be removed, raising the first
+  such error only after the rest are cleaned up. Previously the first failure
+  aborted the loop and left the remaining secret files on disk, which is the
+  outcome `close` exists to prevent. This matches the Go SDK's `firstErr` and the
+  .NET SDK's `firstError`. The Ruby SDK also no longer skips a dangling symlink,
+  which `File.exist?` reports as absent.
 - The `awssm` provider now accepts a trailing slash in `?prefix=` without
   inserting a second slash into the AWS secret name. For example,
   `?prefix=myteam/` resolves to `myteam/secretspec/...`, matching

secretspec-py/secretspec/__init__.py

@@ -138,13 +138,25 @@ def close(self) -> None:
         after resolve returns; the caller owns their lifetime. Call ``close()``
         (or use this object as a context manager) when done so secret files do
         not accumulate in the temp dir. A file already gone is not an error.
+
+        Every file is attempted even if one cannot be removed; the first such
+        error is re-raised once the rest have been cleaned up. Stopping at the
+        first failure would leave the remaining secrets on disk, which is the
+        one outcome this method exists to prevent. Matches the Go SDK's
+        ``firstErr`` and the .NET SDK's ``firstError``.
         """
+        first_error: Optional[OSError] = None
         for secret in self.secrets.values():
             if secret.as_path and secret.path is not None:
                 try:
                     os.remove(secret.path)
                 except FileNotFoundError:
                     pass
+                except OSError as error:
+                    if first_error is None:
+                        first_error = error
+        if first_error is not None:
+            raise first_error
 
     def __enter__(self) -> "Resolved":
         return self

secretspec-py/tests/test_close.py

@@ -0,0 +1,91 @@
+"""``Resolved.close()`` must attempt every ``as_path`` file.
+
+The method exists so secret-bearing temp files do not outlive the result.
+Stopping at the first file the OS refuses to remove leaves every later secret
+on disk — the exact outcome it is meant to prevent — and the caller has no way
+to know which ones survived.
+
+The Go SDK (``firstErr``) and the .NET SDK (``firstError``) already clean up
+everything and report the first failure afterwards; .NET catches ``IOException``
+specifically, which is the ordinary Windows sharing violation raised when
+another process still holds the file open. These tests hold the Python SDK to
+the same contract.
+"""
+
+import os
+from unittest import mock
+
+import pytest
+
+from secretspec import Resolved, ResolvedSecret
+
+
+def _resolved(tmp_path, count=3):
+    """A Resolved over `count` real as_path files."""
+    paths = []
+    for i in range(count):
+        path = tmp_path / f"secret{i}"
+        path.write_text("super-secret-value")
+        paths.append(str(path))
+    secrets = {
+        f"S{i}": ResolvedSecret(
+            value=None, path=path, as_path=True,
+            source="provider", source_provider="dotenv",
+        )
+        for i, path in enumerate(paths)
+    }
+    return Resolved(provider="dotenv", profile="default", secrets=secrets), paths
+
+
+def test_close_removes_every_as_path_file(tmp_path):
+    resolved, paths = _resolved(tmp_path)
+    resolved.close()
+    assert [os.path.exists(p) for p in paths] == [False, False, False]
+
+
+def test_close_is_idempotent(tmp_path):
+    resolved, _ = _resolved(tmp_path)
+    resolved.close()
+    resolved.close()  # a file already gone is not an error
+
+
+def test_close_removes_the_rest_when_one_file_cannot_be_removed(tmp_path):
+    """The regression: one refusal must not strand the other secrets."""
+    resolved, paths = _resolved(tmp_path)
+    blocked = paths[1]
+
+    real_remove = os.remove
+
+    def refuse_one(path, *args, **kwargs):
+        if path == blocked:
+            raise PermissionError(13, "Permission denied", path)
+        return real_remove(path, *args, **kwargs)
+
+    with mock.patch("os.remove", side_effect=refuse_one):
+        with pytest.raises(PermissionError):
+            resolved.close()
+
+    assert not os.path.exists(paths[0]), "file before the failure was not removed"
+    assert not os.path.exists(paths[2]), "file after the failure was stranded on disk"
+    assert os.path.exists(blocked), "the blocked file should still be there"
+
+
+def test_close_reports_the_first_failure(tmp_path):
+    """Two refusals: the first is raised, matching Go's firstErr / .NET's firstError."""
+    resolved, paths = _resolved(tmp_path, count=2)
+
+    def refuse_all(path, *args, **kwargs):
+        raise PermissionError(13, "Permission denied", path)
+
+    with mock.patch("os.remove", side_effect=refuse_all):
+        with pytest.raises(PermissionError) as excinfo:
+            resolved.close()
+
+    assert excinfo.value.filename == paths[0]
+
+
+def test_context_manager_exit_closes(tmp_path):
+    resolved, paths = _resolved(tmp_path)
+    with resolved:
+        assert all(os.path.exists(p) for p in paths)
+    assert not any(os.path.exists(p) for p in paths)

secretspec-rb/lib/secretspec.rb

@@ -87,12 +87,27 @@ def fields
     # block to Builder#load, which closes automatically) when done so secret
     # files do not accumulate in the temp dir. A file already gone is not an
     # error.
+    #
+    # Every file is attempted even if one cannot be removed; the first such
+    # error is re-raised once the rest have been cleaned up. Stopping at the
+    # first failure would leave the remaining secrets on disk, which is the one
+    # outcome this method exists to prevent. Matches the Go SDK's firstErr and
+    # the .NET SDK's firstError.
     def close
+      first_error = nil
       secrets.each_value do |secret|
         next unless secret.as_path && secret.path
 
-        File.delete(secret.path) if File.exist?(secret.path)
+        begin
+          File.delete(secret.path)
+        rescue Errno::ENOENT
+          # already gone
+        rescue SystemCallError => e
+          first_error ||= e
+        end
       end
+      raise first_error if first_error
+
       nil
     end
   end

secretspec-rb/test/test_close.rb

@@ -0,0 +1,90 @@
+# frozen_string_literal: true
+
+# Resolved#close must attempt every as_path file.
+#
+# The method exists so secret-bearing temp files do not outlive the result.
+# Stopping at the first file the OS refuses to remove leaves every later secret
+# on disk -- the exact outcome it is meant to prevent -- and the caller has no
+# way to know which ones survived.
+#
+# The Go SDK (firstErr) and the .NET SDK (firstError) already clean up
+# everything and report the first failure afterwards; .NET catches IOException
+# specifically, which is the ordinary Windows sharing violation raised when
+# another process still holds the file open. These tests hold the Ruby SDK to
+# the same contract.
+
+require "tmpdir"
+require "minitest/autorun"
+
+def ensure_ext
+  pkg = File.expand_path("..", __dir__)
+  return unless Dir[File.join(pkg, "lib", "secretspec", "secretspec_ext.{so,bundle}")].empty?
+
+  system("bash", File.join(pkg, "scripts", "build-ext.sh")) || raise("build-ext.sh failed")
+end
+
+ensure_ext
+require_relative "../lib/secretspec"
+
+class TestClose < Minitest::Test
+  def setup
+    @dir = Dir.mktmpdir("secretspec-close")
+  end
+
+  def teardown
+    FileUtils.rm_rf(@dir) if @dir
+  end
+
+  # A Resolved over `count` real as_path files.
+  def build(count = 3)
+    paths = (0...count).map do |i|
+      path = File.join(@dir, "secret#{i}")
+      File.write(path, "super-secret-value")
+      path
+    end
+    secrets = {}
+    paths.each_with_index do |path, i|
+      secrets["S#{i}"] = Secretspec::ResolvedSecret.new(nil, path, true, "provider", "dotenv")
+    end
+    [Secretspec::Resolved.new("dotenv", "default", secrets, [], nil), paths]
+  end
+
+  def test_close_removes_every_as_path_file
+    resolved, paths = build
+    resolved.close
+    paths.each { |path| refute File.exist?(path), "#{path} was left behind" }
+  end
+
+  def test_close_is_idempotent
+    resolved, = build
+    resolved.close
+    resolved.close # a file already gone is not an error
+  end
+
+  # The regression: one refusal must not strand the other secrets.
+  def test_close_removes_the_rest_when_one_file_cannot_be_removed
+    resolved, paths = build
+    blocked = paths[1]
+
+    File.singleton_class.prepend(Module.new do
+      define_method(:delete) do |*args|
+        raise Errno::EACCES, args.first if args.first == blocked
+
+        super(*args)
+      end
+    end)
+
+    assert_raises(Errno::EACCES) { resolved.close }
+
+    refute File.exist?(paths[0]), "file before the failure was not removed"
+    refute File.exist?(paths[2]), "file after the failure was stranded on disk"
+    assert File.exist?(blocked), "the blocked file should still be there"
+  end
+
+  def test_close_closes_from_the_load_block
+    resolved, paths = build
+    # Builder#load closes automatically when given a block; close is what it calls.
+    resolved.close
+    paths.each { |path| refute File.exist?(path) }
+  end
+end