fix(tae): return open errors from storage init

[jiangxinmeng1]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #25677

What this PR does / why we need it:

• change rpc.NewTAEHandle to return TAE open errors instead of panicking
• propagate TAE open failures through taestorage.NewTAEStorage
• close an already-opened TAE handle if later storage initialization fails
• add unhappy-path tests for open failures and cleanup after logtail server creation errors

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

The panic-to-error direction is correct, but the initialization failure path is not ownership- or generation-safe yet. I found two blockers.

1. A transient logtail-server creation failure permanently closes the QueryClient reused by the retry loop.

newTAEStorage rolls back by calling taeHandler.HandleClose(ctx), and HandleClose closes both the TAE DB and h.client. However, that QueryClient is created and owned by tnservice.store; every createReplica retry passes the same s.queryClient. After the first post-open initialization failure, the retry therefore receives an already-closed client. The storage may subsequently start, but operations using that client will return client-closed errors.

Please make ownership explicit: the handle should close resources it creates (the DB), while the store should close its shared QueryClient exactly once during store shutdown. Add a regression test proving that a failed storage attempt closes the DB without closing the caller-owned client, and that the same client remains usable by the retry.

2. The failed TAE generation is published globally before storage initialization commits.

NewTAEHandle calls RegisterManifestHTTP(tae) immediately after opening the DB. RegisterManifestHTTP uses sync.Once and permanently captures that first DB pointer. If logtail-server creation then fails, this PR closes the DB, but the HTTP handler still points to the closed generation and a later successful retry cannot replace it.

I reproduced this deterministically: inject the logtail-server factory failure, let newTAEStorage return, then serve GET /debug/tae/manifest?table=db.tbl through the default mux. It panics with a nil-pointer dereference because parseTableTarget ignores the failed StartTxn result and dereferences the nil transaction.

Please publish the active DB only after the whole storage constructor succeeds, and make the handler generation-aware (atomically replace/clear the active DB without allowing an old close to clear a newer generation). parseTableTarget should also handle StartTxn errors rather than panic. Tests should cover failed initialization, retry to a new generation, endpoint access between/after generations, and stale-generation cleanup.

The changed package tests and go vet pass locally, but the current tests use a nil QueryClient and only prove that the database directory can be reopened, so they do not exercise either ownership or generation boundary.

[XuPeng-SH]

The previous QueryClient premature-close and stale manifest-generation blockers are fixed, but the ownership transfer is still incomplete and now leaks the QueryClient on store shutdown.

HandleClose correctly no longer closes h.client, because the handle does not own the client. However, that client is created by tnservice.store via client.NewQueryClient, and store.Close() still never calls s.queryClient.Close(). queryService.Close() is unrelated—it only closes the server-side handler. Consequently, the underlying morpc client remains registered in globalClientGC.clients, its backends are not closed, its stopper is not stopped, and the active-client gauge is not decremented. Repeated store lifecycles therefore retain clients/backends in the global manager.

Please complete the ownership move by closing the shared QueryClient exactly once from the store shutdown path, after replica users have stopped, and include its error in the aggregate close result. Add a lifecycle regression proving that closing/retrying an individual TAE storage does not close the shared client, while closing the owning store closes it once.

The new TAE open-error, cleanup, and manifest-generation tests pass locally on this head. I found no additional correctness or performance blocker: manifest dispatch adds only an atomic load, and the reviewed wait paths introduce no new hang risk.

[XuPeng-SH]

The previous QueryClient ownership/leak and manifest-generation blockers are fixed, and the production NewTAEStorage path now propagates open errors correctly. One blocking contract issue remains: the canonical exported rpc.NewTAEHandle still converts newTAEHandle errors into a panic.

This is the exact API and failure mode reported by #25677, and it also contradicts this PR's description (“change rpc.NewTAEHandle to return TAE open errors instead of panicking”). The new regression only calls the sibling NewTAEHandleWithError, so it bypasses the original reproducer. I reran that reproducer against the current head with /dev/null/cannot-create-tae-store; NewTAEHandle still panics 3/3. Any current or future caller choosing the canonical exported constructor therefore retains the process-crash behavior this PR claims to remove.

Please make NewTAEHandle itself return (*Handle, error) and update its callers/tests. If a panic convenience is genuinely required, expose it explicitly as a MustNewTAEHandle-style API rather than leaving the canonical constructor unsafe. The regression should invoke NewTAEHandle on an invalid path and assert error return plus no panic.

Non-blocking cleanup: after the merge from main, queryClientFactory has no assignment/use outside NewService; the stronger branch-local shutdown test was replaced by main's existing test. Please remove this now-dead production test seam unless it is used by a retained lifecycle test.

Local validation otherwise passed: focused unhappy-path tests x5, focused race tests, all four changed packages, build, vet, diff check, and a clean virtual merge with latest main.