Open file handles spec is flaky

[dlpierce]

valkyrie/lib/valkyrie/specs/shared_specs/storage_adapter.rb

Lines 55 to 72 in 814494d

	it "doesn't leave a file handle open on upload/find_by" do
	# No file handle left open from upload.
	resource = Valkyrie::Specs::CustomResource.new(id: "testdiscovery")
	pre_open_files = open_files
	uploaded_file = storage_adapter.upload(file: file, original_filename: 'foo.jpg', resource: resource, fake_upload_argument: true)
	file.close
	expect(pre_open_files.size).to eq open_files.size
	# No file handle left open from find_by
	pre_open_files = open_files
	the_file = storage_adapter.find_by(id: uploaded_file.id)
	expect(the_file).to be_kind_of Valkyrie::StorageAdapter::File
	expect(pre_open_files.size).to eq open_files.size
	end
	def open_files
	`lsof +D .`.split("\n").map { |r| r.split("\t").last }
	end

This spec frequently fails, occasionally even during CI. This is also true for downstream application's custom storage adapters since this is a shared spec.

Currently the check uses lsof +D to list all open files within the application directory. Issues with this include:

• lsof is an additional system level dependency
• If a storage adapter stashes files in a location outside the application dir tree (e.g. /tmp) then those would be missed.
• Other (even non-ruby) processes are included in the open file list.
• lsof itself notes that the +D mode is inefficient and resource hungry.

Potential actions:

• Filter open_files to those with a PID that matches Process.pid
• Use a more specific path
• Include additional paths like /tmp
• Remove the spec or make it a warning?

[tpendragon]

Ooh, I like the pid idea.

I've thought about this one a lot too, since it fails for me locally basically always. We were just leaking SO MANY handles before this test, so I'm scared to remove it.

[dlpierce]

Yeah, I think it's a valuable test to have.
I've been pondering how it would fail in a CI situation where there shouldn't be other processes. Maybe if there is an open file that is missing a corresponding close then if garbage collection doesn't trigger during the spec it would show as a spec failure? We could explicitly garbage collect in open_files I guess? Or maybe we want to be identifying these problems better if that is really what's happening.