feat: enhance websocket connection and support stmt2 on Websocket

[huskar-t]

Description

feat: enhance websocket connection and support stmt2 on Websocket

Issue(s)

• resolve: https://project.feishu.cn/taosdata_td/feature/detail/6664967521

Checklist

Please check the items in the checklist if applicable.

• Is the user manual updated?
• Are the test cases passed and automated?
• Is there no significant decrease in test coverage?

[Copilot]

Pull request overview

Copilot reviewed 121 out of 220 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 122 out of 228 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

ws/client/conn.go:130

• ReadPump/WritePump dereference c.conn without guarding against nil. Since NewClient(nil, ...) is possible (and used in tests), calling pumps on such a client will panic. If nil connections are supported for test/placeholder clients, add early returns (or a clear error) at the beginning of ReadPump/WritePump when c.conn == nil.

func (c *Client) ReadPump() {

ws/client/conn.go:165

• ReadPump/WritePump dereference c.conn without guarding against nil. Since NewClient(nil, ...) is possible (and used in tests), calling pumps on such a client will panic. If nil connections are supported for test/placeholder clients, add early returns (or a clear error) at the beginning of ReadPump/WritePump when c.conn == nil.

func (c *Client) WritePump() {

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sheyanjie-qq]

Summary

The core change introduces a new ws/unified package implementing Stmt2 over WebSocket, converts
ws/stmt, ws/schemaless, etc. into compatibility shims, and adds DECIMAL/DECIMAL64 and BLOB
binary serialization support to common/stmt/stmt2.go. The overall architecture is sound, but
there are several issues that need to be addressed.

---

🔴 Critical

1. common/stmt/mem.go — //go:linkname into runtime internals + no bounds checking

//go:noescape
func memmove(to, from unsafe.Pointer, n uintptr)

//go:linkname memmove runtime.memmove   // ⚠️ links to an internal Go runtime symbol

func Copy(source unsafe.Pointer, data []byte, index int, length int) {
    memmove(unsafe.Pointer(&data[index]), source, uintptr(length))
    // ⚠️ no bounds check: index < 0 or index+length > len(data) silently corrupts memory
}

Issues:

• //go:linkname runtime.memmove is a known anti-pattern. Go 1.23+ tightened restrictions on
  linkname to runtime-internal symbols (see cmd/link: lock down future uses of linkname golang/go#67401); this may break in future Go versions.
• Copy has no bounds checking. A wrong argument silently writes out-of-bounds memory.

Suggestion: All three call sites can be replaced with the standard library. The entire
mem.go + mem.s can be deleted:

// Replace: Copy(unsafe.Pointer(&tableTagLengthList[0]), buffer, tagsLengthOffset, tagsDataLengthLen)
for i, v := range tableTagLengthList {
    binary.LittleEndian.PutUint32(buffer[tagsLengthOffset+i*4:], v)
}

// Replace: Copy(unsafe.Pointer(&tableColLengthList[0]), buffer, colsLengthOffset, colsDataLengthLen)
for i, v := range tableColLengthList {
    binary.LittleEndian.PutUint32(buffer[colsLengthOffset+i*4:], v)
}

// Replace: Copy(unsafe.Pointer(&utf8TableNameLen[0]), buffer, tableNameLengthOffset, tableNameLengthLen)
for i, v := range utf8TableNameLen {
    binary.LittleEndian.PutUint16(buffer[tableNameLengthOffset+i*2:], v)
}

This matches the writeU32 / writeU16 style already used throughout the same file.

---

🟠 Important

2. ws/stmt/rows.go — NewRows silent behavioral breaking change

Original implementation on main:

func NewRows(conn *WSConn, client *client.Client, resp *UseResultResp, timezone *time.Location) *Rows {
    return &Rows{conn: conn, client: client, resultID: resp.ResultID, ...}
    // Next() fetches rows over the network via conn
}

New implementation in this PR:

func NewRows(_ *WSConn, _ *client.Client, resp *UseResultResp, _ *time.Location) *Rows {
    // conn and client are completely ignored
    // Next() always returns io.EOF
}

Any external caller that constructs a Rows via stmt.NewRows(conn, client, resp, tz) and then
calls rows.Next() will now silently get no data with no error returned. A silent behavioral
regression is more dangerous than a compile error.

Suggestion: Make the behavior change explicit in the doc comment:

// NewRows is kept for API compatibility only.
// WARNING: conn and client are ignored. Calling Next() on the returned Rows will
// always return io.EOF. Use stmt.UseResult() to obtain a functional result set.

---

🟡 Minor

4. Copy argument order is the reverse of Go convention

func Copy(source unsafe.Pointer, data []byte, index int, length int)
// Direction: source → data[index:]
// Go convention: copy(dst, src) — destination first

If this function is kept (rather than deleted as suggested above), rename it to something like
writeBytesAt(dst []byte, offset int, src unsafe.Pointer, n int) to make the write direction
unambiguous.

5. ws/unified/stmt.go — Close uses a stale runtime snapshot

func (s *Stmt) Close(reqID int64) error {
    ...
    runtime := s.runtime  // ⚠️ captured at Stmt creation; may be stale after reconnect
    _, _, err := s.client.sendStmtJSONAndDecode(runtime, ...)
}

After a reconnect, s.runtime is updated by reconnectAndInitLocked, but a race could still
cause Close to send to the old connection. The practical impact is limited (Close silently
ignores connection errors), but using s.client.runtimeClient() would be consistent with other
methods.

---

✅ What's done well

1. Error model design — ws/unified/error.go provides ErrorType + Error.Is() +
   IsConnectionRelatedError() helpers that make error classification clean and testable.

2. Reconnect concurrency protection — The reconnectLock + reconnectDone channel pattern
   prevents multiple goroutines from triggering concurrent reconnects and lets waiters reuse the
   result of an in-progress reconnect.

3. Clear deprecation path — All methods in common/param/param.go and ws/stmt/ are
   annotated with Deprecated pointing clearly to the new API (unified.Stmt.Bind).

4. stmtBindMode mutual exclusion — Prevents mixing the compat API
   (SetTableName/SetTags/BindParam) with raw Bind() on the same prepared statement, avoiding
   hard-to-debug state corruption.

5. ws/unified test quality — The package includes failover, reconnect, concurrency, fuzz,
   and regression tests, which is a high bar for a new package of this size.

---

Priority Summary

Priority	File	Action
P0	common/stmt/mem.go + mem.s	Delete; replace with binary.LittleEndian writes
P1	ws/stmt/rows.go	Document that NewRows now returns a non-functional Rows where Next() always returns io.EOF
P2	taosWS/compat_write.go	Add basic tests or document why none are needed
P2	ws/unified/stmt.go Close	Use s.client.runtimeClient() instead of s.runtime

[Copilot]

Pull request overview

Copilot reviewed 125 out of 231 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 127 out of 233 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 127 out of 233 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 127 out of 233 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[huskar-t]

Summary

The core change introduces a new ws/unified package implementing Stmt2 over WebSocket, converts ws/stmt, ws/schemaless, etc. into compatibility shims, and adds DECIMAL/DECIMAL64 and BLOB binary serialization support to common/stmt/stmt2.go. The overall architecture is sound, but there are several issues that need to be addressed.

🔴 Critical

1. common/stmt/mem.go — //go:linkname into runtime internals + no bounds checking

//go:noescape
func memmove(to, from unsafe.Pointer, n uintptr)

//go:linkname memmove runtime.memmove   // ⚠️ links to an internal Go runtime symbol

func Copy(source unsafe.Pointer, data []byte, index int, length int) {
    memmove(unsafe.Pointer(&data[index]), source, uintptr(length))
    // ⚠️ no bounds check: index < 0 or index+length > len(data) silently corrupts memory
}

Issues:

• //go:linkname runtime.memmove is a known anti-pattern. Go 1.23+ tightened restrictions on
  linkname to runtime-internal symbols (see cmd/link: lock down future uses of linkname golang/go#67401); this may break in future Go versions.
• Copy has no bounds checking. A wrong argument silently writes out-of-bounds memory.

Suggestion: All three call sites can be replaced with the standard library. The entire mem.go + mem.s can be deleted:

// Replace: Copy(unsafe.Pointer(&tableTagLengthList[0]), buffer, tagsLengthOffset, tagsDataLengthLen)
for i, v := range tableTagLengthList {
    binary.LittleEndian.PutUint32(buffer[tagsLengthOffset+i*4:], v)
}

// Replace: Copy(unsafe.Pointer(&tableColLengthList[0]), buffer, colsLengthOffset, colsDataLengthLen)
for i, v := range tableColLengthList {
    binary.LittleEndian.PutUint32(buffer[colsLengthOffset+i*4:], v)
}

// Replace: Copy(unsafe.Pointer(&utf8TableNameLen[0]), buffer, tableNameLengthOffset, tableNameLengthLen)
for i, v := range utf8TableNameLen {
    binary.LittleEndian.PutUint16(buffer[tableNameLengthOffset+i*2:], v)
}

This matches the writeU32 / writeU16 style already used throughout the same file.

🟠 Important

2. ws/stmt/rows.go — NewRows silent behavioral breaking change

Original implementation on main:

func NewRows(conn *WSConn, client *client.Client, resp *UseResultResp, timezone *time.Location) *Rows {
    return &Rows{conn: conn, client: client, resultID: resp.ResultID, ...}
    // Next() fetches rows over the network via conn
}

New implementation in this PR:

func NewRows(_ *WSConn, _ *client.Client, resp *UseResultResp, _ *time.Location) *Rows {
    // conn and client are completely ignored
    // Next() always returns io.EOF
}

Any external caller that constructs a Rows via stmt.NewRows(conn, client, resp, tz) and then calls rows.Next() will now silently get no data with no error returned. A silent behavioral regression is more dangerous than a compile error.

Suggestion: Make the behavior change explicit in the doc comment:

// NewRows is kept for API compatibility only.
// WARNING: conn and client are ignored. Calling Next() on the returned Rows will
// always return io.EOF. Use stmt.UseResult() to obtain a functional result set.

🟡 Minor

4. Copy argument order is the reverse of Go convention

func Copy(source unsafe.Pointer, data []byte, index int, length int)
// Direction: source → data[index:]
// Go convention: copy(dst, src) — destination first

If this function is kept (rather than deleted as suggested above), rename it to something like writeBytesAt(dst []byte, offset int, src unsafe.Pointer, n int) to make the write direction unambiguous.

5. ws/unified/stmt.go — Close uses a stale runtime snapshot

func (s *Stmt) Close(reqID int64) error {
    ...
    runtime := s.runtime  // ⚠️ captured at Stmt creation; may be stale after reconnect
    _, _, err := s.client.sendStmtJSONAndDecode(runtime, ...)
}

After a reconnect, s.runtime is updated by reconnectAndInitLocked, but a race could still cause Close to send to the old connection. The practical impact is limited (Close silently ignores connection errors), but using s.client.runtimeClient() would be consistent with other methods.

✅ What's done well

1. Error model design — ws/unified/error.go provides ErrorType + Error.Is() +
   IsConnectionRelatedError() helpers that make error classification clean and testable.
2. Reconnect concurrency protection — The reconnectLock + reconnectDone channel pattern
   prevents multiple goroutines from triggering concurrent reconnects and lets waiters reuse the
   result of an in-progress reconnect.
3. Clear deprecation path — All methods in common/param/param.go and ws/stmt/ are
   annotated with Deprecated pointing clearly to the new API (unified.Stmt.Bind).
4. stmtBindMode mutual exclusion — Prevents mixing the compat API
   (SetTableName/SetTags/BindParam) with raw Bind() on the same prepared statement, avoiding
   hard-to-debug state corruption.
5. ws/unified test quality — The package includes failover, reconnect, concurrency, fuzz,
   and regression tests, which is a high bar for a new package of this size.

Priority Summary

Priority File Action
P0 common/stmt/mem.go + mem.s Delete; replace with binary.LittleEndian writes
P1 ws/stmt/rows.go Document that NewRows now returns a non-functional Rows where Next() always returns io.EOF
P2 taosWS/compat_write.go Add basic tests or document why none are needed
P2 ws/unified/stmt.go Close Use s.client.runtimeClient() instead of s.runtime

Point 1: common/stmt/mem.go — //go:linkname + no bounds checking

✅ Fixed.

• Removed //go:linkname runtime.memmove, //go:noescape, and the empty mem.s assembly stub
• Replaced Copy() with type-safe copyUint32SliceToBytes(dest, source) / copyUint16SliceToBytes(dest, source) using
  Go's standard copy() with bounds-checked slicing
• Split by build tags: go1.22+ uses unsafe.Slice (per-file language version), <go1.22 uses manual slice struct
• All three call sites in stmt2.go updated; unsafe import removed
• Same treatment applied to common/parser

Point 2: ws/stmt/rows.go — NewRows silent behavioral breaking change

⚠️ Valid concern, but already addressed.

NewRows indeed no longer fetches row data — this is by design, not an oversight:

• Marked Deprecated with explicit doc: "compatibility-only metadata wrapper; it no longer fetches row data"
• All methods implement dual paths: delegate to unified.ResultSet when available, return compatible metadata or
  io.EOF otherwise
• Functional result sets are constructed internally via newRowsFromResultSet(); callers should use stmt.UseResult()

Point 4: Copy argument order is the reverse of Go convention

✅ Naturally resolved by Point 1 fix (for common/stmt).

Copy(source, data, index, length) has been removed. The new functions copyUint32SliceToBytes(dest, source) /
copyUint16SliceToBytes(dest, source) follow Go's destination-first convention consistent with copy(dst, src).

The Copy signature in common/parser is retained for external API compatibility and is out of scope for this PR.

Point 5: ws/unified/stmt.go — Close uses a stale runtime snapshot

❌ Not a real issue. The current design is correct.

Close() sets closed = true and captures stmtID under lock, then reads s.runtime after unlock. The reviewer's concern
is that a reconnect could make the runtime stale, but:

1. After closed = true, all operation paths (prepareWithReconnectLocked, execWithReconnectLocked, etc.) check
   closed first — reconnect cannot happen after Close, so s.runtime will not be updated during Close
2. Even hypothetically, using the old runtime + old stmtID is semantically correct — it closes the old handle on
   the old connection. Using the new runtime would send a close for a non-existent stmtID to the new connection
3. Close already defensively handles connection-related errors (ClosedError / reconnectable error → return nil)

[zitsen]

Code Review Summary

Reviewed: 233 files, 32K+ additions across ws/unified, stmt2, failover, CI workflows, and backward-compatible wrappers.

CI Status: All 20 checks passed ✅ (lint, ASAN, race detector, failover gate, reliability gate, multi-version Go tests)

Findings (non-blocking)

1. Medium – Potential race window in request-response sync (ws/unified/request.go):
   There is a timing window between releasing pendingLock (after registering a pending request) and calling runtime.Send(). If swapRuntime() executes in this window, the send could go to a stale runtime. Consider verifying runtime generation after Send() completes, or restructuring to ensure atomic registration+send under the same generation guarantee. The race detector CI passing suggests this is unlikely in practice, but worth hardening in a follow-up.

2. Medium – Integer overflow risk in stmt2 binary marshalling (common/stmt/stmt2.go):
   The ~600-line marshalStmt2BinaryWithHeader performs cumulative uint32 additions for buffer sizes. With many tables or large variable-length fields, these could theoretically overflow. Some bounds checks exist (e.g., table name length vs math.MaxUint16), but comprehensive overflow protection for cumulative buffer size calculations would be a good hardening.

Verdict

No critical issues found. The architecture is solid — unified client with generation-based failover, proper mutex protection, and comprehensive CI coverage including ASAN and race detection. Approving.