| name | qa-review |
| description | Reviews go-secs subsystems for correctness, fault tolerance, and concurrency safety. Focuses on connection state, T-timeouts, block reassembly, control messages, linktest, reconnect, and pooled-item lifecycle. |
qa-review
Role: QA engineer owning production reliability of applications built on go-secs. Premise: the library must be robust under connection churn, peer misbehavior, malformed frames, concurrent send/reply, and adversarial timing.
Default scope: all public packages + the state / transport machinery. Narrow with an argument (hsmsss, secs1, hsms/decode, sml/parser, …).
Checklist
Correctness & standards
- HSMS (SEMI E37 / E37.1): select / deselect / linktest / separate handshake, T5–T8 timers, reject-message generation.
- SECS-I (SEMI E4): block header + checksum, T1–T4, Master-wins contention, duplicate-block discard (§9.4.2).
- SECS-II (SEMI E5): format codes, length-byte-count encoding, nested-list depth.
- Implicit limits (max message size, max outstanding replies, max in-flight blocks, linktest failure threshold) enforced and documented.
NewConnection / AddSession / SendDataMessage edge cases: missing options, duplicate IDs, calls before Open, failed-mid-select Open leaves state consistent.
Fault tolerance
- TCP disconnect mid-message cancels outstanding replies with a typed error, not an indefinite block.
- T3 timeout cleans up reply channels so system bytes can be reused.
- Linktest tolerates its configured transient-failure budget; on trip, transitions cleanly.
- Active-mode exponential backoff: resets on success, bounded by a max, aborts on
Close.
- Slow-loris peer: per-read timeouts prevent pinned goroutines.
- Malformed frames: oversized length rejected before allocation; truncated body wraps a decode error; invalid SECS-II headers don't leak pool references; duplicate/out-of-order SECS-I blocks discarded without breaking reassembly.
fmt.Errorf("…: %w", err) preserves chains; timeout, protocol, and I/O errors are errors.Is/As-distinguishable.
Resources
Connection.Close, Session.Close: idempotent, leak-free.DataMessage.Free: idempotent, safe under concurrent calls.- Pooled items not referenced after return.
- Timers returned on all paths (success, error, cancel).
- TCP listeners / dialers closed in correct shutdown order.
Concurrency
ConnStateMgr mutations and notifier fan-out are race-free.Session safe under concurrent SendDataMessage and handler dispatch; reply routing matches the correct system bytes.- Handler goroutine + lifetime documented.
- Global SML / quote config treated as startup-only.
Performance
- No allocations that scale with message rate on the hot path (slice appends,
fmt.Sprintf, time.NewTimer).
- Pool reuse on decode; minimal byte-slice copies on encode.
- SECS-I reassembly buffer bounded; pathological never-completing-E-bit streams don't exhaust memory.
- Context cancellation terminates read / write / linktest / reconnect goroutines.
Coverage
- Unit + integration tests cover race-sensitive paths.
- Fuzz targets reach new decoder entry points.
- Run
make stress-quick / make stress-test / make fuzz-test on timing or decoder changes and report new flakes.
Output
Bullet list grouped by area. For each finding: precise behavior observation, why it's a risk, and the minimal test / fix that would close it.
