Free Arrow IPC resources from one defer, not two call sites (#733)

[xe-nvdk]

Fixes #733.

What was wrong, and what was not

The Arrow IPC writer released its DuckDB reader, pooled connection and timeout context from two places: straight-line at the end of the writer, and again from safeStream's onPanic. A panic after the straight-line call, in the trailer or logging statements that follow it, ran the whole cleanup a second time.

I filed #733 claiming this over-releases the reader and logs a spurious cleanup panic. Both are false, and I have corrected the issue. Measured rather than assumed:

• duckdb-go's recordReader.Release opens with an explicit refCount <= 0 early return, and upstream pins that with a test.
• *sql.Conn.Close returns sql.ErrConnDone, which the helper discards.
• cancel() is idempotent by contract.

Injecting a panic into the writer's final log statement reproduces the double call and produces no cleanup panic, no over-release, and no leak.

Why it is still worth changing

The code was correct only because all three concrete resources tolerate a second call. The array.RecordReader interface promises nothing of the sort and no test pinned it.

Nor would losing the invariant have failed anything. arrow-go guards over-release behind debug.Assert, which is a no-op unless built with -tags assert, and no Arc build sets it, so a second Release would take the refcount from 0 to -1 and simply not re-run the cleanup branch. Nothing corrupts and nothing reports. An invariant with no signal behind it has to be structural rather than conventional.

Cleanup is now a single defer registered before anything in the writer that can panic. It runs exactly once whether the writer returns normally or unwinds, because a defer inside sw runs during unwinding, ahead of safeStream's recover.

This is what every other stream writer in the package already does (query.go x3, query_arrow_json.go, query_msgpack.go all defer their release at closure entry and leave onPanic for disposition only). Arrow IPC was the outlier; it no longer is.

Honest scope of the benefit

It also covers a panic raised inside onPanic ahead of where the release used to sit, which skipped it and stranded the pooled connection. That one is reasoned, not tested, and the code says so: nothing in onPanic can be made to panic from a test without adding a seam purely for it.

An earlier draft of this change also advertised runtime.Goexit as a fixed leak. I dropped the claim after trying to test it: it is unreachable on that goroutine outside testing, and forcing it hangs the request regardless, because NewStreamReader never reaches pw.Close(). Claiming an untestable benefit in release notes is the thing this PR's own commit message criticises, so it is gone rather than hand-waved.

Not strictly better on every path, and the comment says so. releaseArrowStreamResources installs its recover first, so a panic in reader.Release skipped the conn.Close and cancel below it, and a later panic used to retry them through onPanic. That retry is gone. Exotic enough not to be worth restoring, but it is a real direction where the old shape did more.

Timing

Release now happens after the trailer set and the final log line rather than before them. Those are counter increments, an in-memory map write and one zerolog event, and query_arrow_json.go already holds its connection across the same work.

Testing

New TestExecuteQueryArrowReleasesExactlyOnce asserts exactly one cleanup on the normal and panic paths, through a releaseArrowStreamResourcesFunc seam added alongside the existing streamArrowIPCFunc one. The panic is injected with a zerolog hook keyed on the final log message, which is the only point landing after the old straight-line release.

Verified red on 17aaaef: cleanup ran 2 times, want exactly 1. Green here. go build, go vet, go test -race ./internal/api/ all clean, and the CI panic-safety guard passes.

Stale docs corrected rather than left

Four, all invalidated by this change and none of which the diff could leave honest:

• releaseArrowStreamResources no longer claims to be called from the panic path; it has one caller covering both. The note explains why its internal recover must survive anyway.
• safeStream no longer says onPanic is for releasing resources, since no caller does that now. Verified the rewritten wording describes all six current callers.
• The Panic in the Arrow IPC stream writer leaks the reader, connection and timeout context #716 regression test named "straight-line code" as its discriminator; it is now the deferred call.
• The Panic in the Arrow IPC stream writer leaks the reader, connection and timeout context #716 and Eight streaming response writers have no panic recovery, so a panic there crashes the process #717 release-notes entries described the shape this replaces, both unreleased, so they point forward to the new entry rather than contradicting it.