feat: infrastructure connectors with lifecycle management#18
feat: infrastructure connectors with lifecycle management#18
Conversation
Connect[T] creates infrastructure clients via factory, registers with slush, emits capitan signals, and tracks io.Closer instances. Shutdown closes infrastructure in reverse connection order with signal emission. Part 2 of #14 — infrastructure connectors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wintermute-zbz
left a comment
wintermute-zbz
left a comment
There was a problem hiding this comment.
Review — #18
Verdict: APPROVE
Connect[T] with lifecycle management. Reverse-order shutdown. Capitan signal emission.
Findings
service.go:130-145 — closer errors are signaled but not returned.
During shutdown, Close() failures emit capitan.Error signals but don't propagate to the caller. Only the engine shutdown error returns. For graceful shutdown this is defensible — the process is exiting, observability matters more than propagation. But if a caller ever needs to know whether cleanup succeeded, they have no way to find out. Consider errors.Join. Non-blocking — design call.
connector_test.go:159,189 — time.Sleep(100ms) for server readiness.
Inherently racy. Works in practice because rocco.Engine.Start binds before blocking. A ready-channel would be deterministic. Non-blocking — low probability.
Assessment
- Shutdown order correct: stop accepting requests, then close infrastructure in reverse order.
svc()panic inConnectconsistent with existing singleton pattern.- Closer slice copied under RLock, iterated outside lock. No lock held during slow I/O.
io.Closerdetection via type assertion is correct for generics.- Reset correctly nils closers slice.
- No new dependencies — capitan already in go.mod.
- Test coverage thorough: connect, factory error, closer tracking, no-closer exclusion, signal emission, reverse-order shutdown, empty shutdown.
Clean.
| Register[T](k, client) | ||
| capitan.Info(ctx, SignalConnected, | ||
| KeyConnectorName.Field(name), | ||
| KeyConnectorType.Field(reflect.TypeOf(client).String()), |
There was a problem hiding this comment.
same concern as the other PR - reflection should pass through sentinel so we capture metadata. it may even already exist - use sentinel.Inspect
There was a problem hiding this comment.
Fixed — using sentinel.TryInspectT with reflect fallback for non-struct types. Connect's T can be interfaces or pointers to external structs, so TryInspect is the safe variant here.
Replaces reflect.TypeOf(client).String() with sentinel.TryInspect[T]() for struct types, falling back to reflect for non-structs. Consistent with the sentinel-first approach across sum. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wintermute-zbz
left a comment
wintermute-zbz
left a comment
There was a problem hiding this comment.
Re-review — #18
Verdict: APPROVE
New commit adds sentinel.TryInspect[T]() with reflect.TypeOf fallback for connector type metadata. Right call — Connect[T] accepts external types (database drivers, etc.) that sentinel may not have metadata for, unlike Config[T] which only works with user-defined structs.
reflect import remains (needed for the fallback). Previous findings unchanged. Tests pass.
reflect import was unnecessary — sentinel.TryInspect covers structs, connector name covers everything else. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reorder namedCloser and Service fields to minimize pointer bytes scanned by GC. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary
Connect[T]— creates infrastructure clients via factory, registers with slush, emits capitan signals, tracksio.Closerfor shutdownSignalConnected,SignalDisconnectedwithKeyConnectorName,KeyConnectorType,KeyConnectorErrorfield keysPart 2 of #14 — infrastructure connectors.
Test plan
🤖 Generated with Claude Code