fix(collection): call upstream close() and keep the handle alive when destroy() fails (#222)

[s2x]

Closes #222. Blocks #226, which relies on close() and destroy() reporting FAILED_PRECONDITION while an iterator is open.

Two defects

1. close() never called upstream close()

ZVec::close() only dropped our reference from the global collections registry. The real close — flush, release open files, release the collection lock — ran later inside the upstream destructor, and its Status was discarded. So writes were not guaranteed to be on disk when close() returned, and close errors were invisible. The docblock already claimed it could throw ZVecException, which it never could.

It now calls upstream Collection::close(), new in v0.7.0. The outcome decides the object state:

outcome	behaviour
FAILED_PRECONDITION (5, e.g. open iterators)	upstream changed nothing → throw, keep the registry entry, keep the object open and usable
any other error	upstream has already released its resources (close_locked() releases even when the final flush fails) → free the handle, mark closed, then throw
OK	free the handle, mark closed

close() stays idempotent. __destruct() no longer propagates — PHP turns an exception raised during shutdown into a fatal error — so on failure it falls back to dropping the registry reference, which is the only way to reclaim the C++ object.

2. A failed destroy() freed the native collection

zvec_collection_destroy() erased the registry entry unconditionally, even when upstream returned an error. That deleted the C++ Collection while the PHP object still held the raw pointer, so the next method call used freed memory.

Reproduced before the fix by destroying a read-only collection, which upstream rejects:

destroy rejected: INVALID_ARGUMENT
fetch after failed destroy: 1        # was: "ZVecException: collection is already closed."

The entry is now erased only on success. On failure upstream leaves the collection untouched, so the handle stays valid — and destroy() throwing from checkStatus() before touching $this->closed already left the object open and usable. No PHP logic change was needed in destroy(), only the comment on its closed-branch catch, which claimed it prevented an "orphaned" C++ object. Before the fix the erase always happened, so that call was dead code.

A note on upstream's own C API

zvec_collection_close() deliberately does not mirror upstream's C API, where the function of the same name only deletes the handle and never calls Collection::close(). We follow the Python SDK, which calls the real close and raises on error — otherwise there would be no status to report and #226's guard could not work.

Verification

count=1 id=1                                       # close() flushed with no explicit flush()
second close: ok                                   # idempotent
closed: Collection is closed. Open with ZVec::open() to continue.
close after destroy: ok
destructor flushed: 7

Full suite:

Number of tests : 204               204
Tests skipped   :   0 (  0.0%)
Tests failed    :   0 (  0.0%)
Tests passed    : 202 ( 99.0%)

test_lifecycle_close_twice, test_close_vs_destroy, test_closed_collection_protection, test_lifecycle_destroy_after_close, test_lifecycle_destroy_then_destruct, test_lifecycle_method_on_destroyed and test_collection_destroy all pass unchanged — the new behaviour is additive from their point of view. test_null_handle_collection.phpt gained a null case, test_ffi_load.phpt the new symbol.

AGENTS.md's Collection Lifecycle section now states the rule that caused defect 2: do not free the handle before the status is known.

Note

The FAILED_PRECONDITION branch of close() is implemented and reasoned about, but not yet reachable by a test — it needs an open document iterator, which is #226. The test for it lands there. The other three branches are covered now.

🤖 Generated with Claude Code