basecamp / basecamp/basecamp-sdk
Event feed: Observer.Disconnected is documented unconditional but four teardown classes are silent
- Dominant language
- Go
- Stars
- 49
- Forks
- 12
- Avg merge
- 20h 47m
- Merged PRs (30d)
- 89
Description
`Observer.Disconnected` is documented as unconditional and is not:
```go
// Disconnected fires when a socket is torn down.
Disconnected func(reason string, err error)
```
`loop.disposeAttempt` is what tears a socket down, and it does not notify — every
caller decides for itself. The failure paths all call `observeDisconnected`
alongside it, but the terminal and clean-exit paths do not, so a host that
releases per-connection state on `Disconnected` never learns those sockets are
gone. The silent disposals, from the Go reference connector
(`go/pkg/basecamp/eventfeed/`):
- clean cancellation / `Connector.Close` (`outcomeClosed`, several sites in
`loop.go` and `catchup.go`)
- consumer break (the `deliver` false-yield paths)
- `reject_subscription` → Terminal(`subscription_rejected`) (`loop.go`)
- the continuation edges → Terminal(`invalid_continuation`) (`catchup.go`,
`recovery.go`)
- `recoverGone`'s Terminal(`feed_gap`) and Terminal(`buffer_overflow`)
Raised by Codex on #705 (thread on `loop.go:1110`). The finding is correct. It is
**not** being fixed in #705, and the reason is that the remedy is not Go-local.
### Why this is a contract question, not a bug fix
`Observer.disconnected` is part of the tier-2 fixture vocabulary —
`conformance/event-feed/schema.json` carries `expectDisconnectedInvalidFrame`,
and the observer sequence a scenario asserts is normative for all six SDKs.
Adding a notification to the terminal paths changes that sequence, so whichever
way it is resolved it has to be resolved in SPEC §23 and applied across the
board, not patched into the Go reference alone.
The question SPEC §23 does not currently answer: **is a terminal disposition a
"teardown" for observability purposes?** Two defensible readings, and they want
different fixes:
1. **Every disposal notifies.** `Disconnected` means "this socket is gone",
full stop; a host releasing per-connection state needs it on every path, and
a terminal is exactly when it most needs it. Fix: move the notification into
`disposeAttempt`, give it the reason/error the caller already has, and audit
for the double-notify that would create on the failure paths.
2. **`Disconnected` is the socket-failure edge only**, and a terminal is
already reported through the terminal element the iteration yields — which
carries strictly more information. Fix: correct the doc comment on
`Observer.Disconnected` to say so, and say where the terminal paths are
observed instead.
Either way there is also the clean-close path, which neither a terminal element
nor a failure edge covers: `Connector.Close` disposes a live socket and reports
nothing at all.
### Scope
- Decide the rule in SPEC §23 ("Consumer Surface" / the observer contract).
- Apply it to the Go reference connector and the other five SDKs' connectors as
they land.
- Extend the tier-2 fixtures so the chosen rule is asserted rather than
assumed — the current schema can express the invalid-frame disconnect but has
no assertion for "a terminal produced exactly one/zero disconnect
notifications".
Follows #606, #614, #645, #696.
Contributor guide
Research direction
Start with SPEC §23 and conformance/event-feed/schema.json to understand the observer contract and existing assertions. Then inspect go/pkg/basecamp/eventfeed/, especially loop.go, catchup.go, and recovery.go, and compare the connector paths across the SDKs. Done means the rule is decided in the spec, implemented consistently, and covered by tier-2 fixtures.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, documentation, testing
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100