Repository: InsightSoftwareConsortium/ITKIOOMEZarrNGFF (language: python)

Issue:
BUG: Zip file output sometimes corrupted on Linux
## Overview

Tests for reading from `.zip` file store sometimes fail. Exact cause is unclear at the moment.

First CI run (failed)
https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/actions/runs/5716549674/job/15488329023?pr=35

Second CI run, no changes (passing):
https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/actions/runs/5716549674/job/15490659684?pr=35

## Workaround

Re-running CI seems to work for now.



Discussion / hints:
This is currently blocking the `v0.1.3` PyPI release: https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/actions/runs/5739761493
Following investigation in https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/pull/44, it was observed that the `memory.zip` output of the `inMemory_zip` test is sometimes a corrupted archive. The exact cause for this is not clear. Behavior is observed regularly in CI and infrequently in local testing on Linux platforms.

The Tensorstore `zip` driver is maintained in the https://github.com/InsightSoftwareConsortium/tensorstore fork and is not maintained by Google. We will need to track this ourselves.

Following offline discussion, we will disable the failing test for now to prevent blocking PyPI releases and continue investigation in parallel with subsequent work.
After all the contents are added to in-memory zip file, the buffer size is 47447. After [`closeZip()`](https://github.com/InsightSoftwareConsortium/tensorstore/blob/afb30a4bcc4a3d93053509efb72edbfcd518e726/tensorstore/kvstore/zip/zip_key_value_store.cc#L148-L175) is called, the buffer size increases to the correct value of 47685. This leads me to suspect that [destructor](https://github.com/InsightSoftwareConsortium/tensorstore/blob/afb30a4bcc4a3d93053509efb72edbfcd518e726/tensorstore/kvstore/zip/zip_key_value_store.cc#L143-L146) is not invoked sometimes.

I will now try to fix this.

I originally wrote this in the wrong issue's discussion: https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/issues/48#issuecomment-1699529683
Looking at tensorstore's [commit log](https://github.com/google/tensorstore/commits/master), there were some fixes recently regarding caches and deep copies. I will rebase our changes on top of current master, to see whether that resolves the problem.
With rebasing, I ran into a snag: https://github.com/google/tensorstore/issues/110.
With building cleared up, the rebase does not help with crashes. We should still use the newer version of tensorstore, so I created this PR:
https://github.com/InsightSoftwareConsortium/tensorstore/pull/1
Please just review, do not click merge - this branch should become the new master.

Fix patch:
diff --git a/include/itkOMEZarrNGFFImageIO.h b/include/itkOMEZarrNGFFImageIO.h
index 941affd..e31d541 100644
--- a/include/itkOMEZarrNGFFImageIO.h
+++ b/include/itkOMEZarrNGFFImageIO.h
@@ -100,6 +100,14 @@ class IOOMEZarrNGFF_EXPORT OMEZarrNGFFImageIO : public ImageIOBase
     size_t size;
   };
 
+  /** Construct a "magic" file name from the provided bufferInfo. */
+  static std::string
+  MakeMemoryFileName(const itk::OMEZarrNGFFImageIO::BufferInfo & bufferInfo)
+  {
+    size_t bufferInfoAddress = reinterpret_cast<size_t>(&bufferInfo);
+    return std::to_string(bufferInfoAddress) + ".memory";
+  }
+
   /*-------- This part of the interfaces deals with reading data. ----- */
 
   /** Determine the file type. Returns true if this ImageIO can read the
@@ -231,6 +239,13 @@ class IOOMEZarrNGFF_EXPORT OMEZarrNGFFImageIO : public ImageIOBase
   int                m_TimeIndex = INVALID_INDEX;
   int                m_ChannelIndex = INVALID_INDEX;
   AxesCollectionType m_StoreAxes;
+
+  // An empty zip file consists of 22 bytes of "end of central directory" record. More:
+  // https://github.com/google/tensorstore/blob/45565464b9f9e2567144d780c3bef365ee3c125a/tensorstore/internal/compression/zip_details.h#L64-L76
+  constexpr static unsigned m_EmptyZipSize = 22;
+  char                      m_EmptyZip[m_EmptyZipSize] = "PK\x05\x06"; // the rest is filled with zeroes
+  const BufferInfo          m_EmptyZipBufferInfo{ m_EmptyZip, m_EmptyZipSize };
+  const std::string         m_EmptyZipFileName = MakeMemoryFileName(m_EmptyZipBufferInfo);
 };
 } // end namespace itk
 
diff --git a/src/itkOMEZarrNGFFImageIO.cxx b/src/itkOMEZarrNGFFImageIO.cxx
index 1cfc8d9..cb55104 100644
--- a/src/itkOMEZarrNGFFImageIO.cxx
+++ b/src/itkOMEZarrNGFFImageIO.cxx
@@ -635,7 +635,8 @@ OMEZarrNGFFImageIO::Read(void * buffer)
   }
 
   if (false)
-  {}
+  {
+  }
   READ_ELEMENT_IF(float)
   READ_ELEMENT_IF(int8_t)
   READ_ELEMENT_IF(uint8_t)
@@ -757,7 +758,10 @@ OMEZarrNGFFImageIO::WriteImageInformation()
 void
 OMEZarrNGFFImageIO::Write(const void * buffer)
 {
-  tsContext = tensorstore::Context::Default(); // start with clean zip handles
+  if (m_FileName.substr(m_FileName.size() - 4) == ".zip" || m_FileName.substr(m_FileName.size() - 7) == ".memory")
+  {
+    tsContext = tensorstore::Context::Default(); // start with clean zip handles
+  }
   this->WriteImageInformation();
 
   if (itkToTensorstoreComponentType(this->GetComponentType()) == tensorstore::dtype_v<void>)
@@ -791,7 +795,8 @@ OMEZarrNGFFImageIO::Write(const void * buffer)
   std::string driver = getKVstoreDriver(this->GetFileName());
 
   if (false) // start with a plain "if"
-  {}         // so element statements can all be "else if"
+  {
+  } // so element statements can all be "else if"
   ELEMENT_WRITE(int8_t)
   ELEMENT_WRITE(uint8_t)
   ELEMENT_WRITE(int16_t)
@@ -807,8 +812,13 @@ OMEZarrNGFFImageIO::Write(const void * buffer)
     itkExceptionMacro("Unsupported component type: " << GetComponentTypeAsString(this->GetComponentType()));
   }
 
-  // Create a new context to close the open zip handles
-  tsContext = tensorstore::Context::Default();
+  if (m_FileName.substr(m_FileName.size() - 4) == ".zip" || m_FileName.substr(m_FileName.size() - 7) == ".memory")
+  {
+    // Attempt to read a non-existent file from the in-memory zip to close the current one
+    nlohmann::json temp;
+    bool           wasRead = jsonRead(m_EmptyZipFileName + "/non-existent.json", temp, "zip");
+    assert(wasRead == false);
+  }
 }
 
 


Test patch:
diff --git a/test/itkOMEZarrNGFFInMemoryTest.cxx b/test/itkOMEZarrNGFFInMemoryTest.cxx
index 0c96b93..f54937d 100644
--- a/test/itkOMEZarrNGFFInMemoryTest.cxx
+++ b/test/itkOMEZarrNGFFInMemoryTest.cxx
@@ -57,8 +57,7 @@ doTest(const char * inputFileName, const char * outputFileName)
 
   itk::OMEZarrNGFFImageIO::BufferInfo bufferInfo{ buffer.data(), buffer.size() };
 
-  size_t      bufferInfoAddress = reinterpret_cast<size_t>(&bufferInfo);
-  std::string memAddress = std::to_string(bufferInfoAddress) + ".memory";
+  std::string memAddress = itk::OMEZarrNGFFImageIO::MakeMemoryFileName(bufferInfo);
 
   reader->SetFileName(memAddress);
   ITK_TRY_EXPECT_NO_EXCEPTION(reader->Update());
@@ -83,6 +82,7 @@ doTest(const char * inputFileName, const char * outputFileName)
   writer->SetImageIO(zarrIO);
   ITK_TRY_EXPECT_NO_EXCEPTION(writer->Update());
 
+  // Write zip bitstream to disk
   std::ofstream oFile(outputFileName, std::ios::binary);
   oFile.write(bufferInfo.pointer, bufferInfo.size);
   free(bufferInfo.pointer);