basecamp / basecamp/basecamp-sdk

Event feed: Close called from an Observer or SignalHandler does not preempt what runs after it (six sites)

Open
#759 1 comment 0 reactions 0 assignees View on GitHub
bug go spec
Dominant language
Go
Stars
49
Forks
12
Avg merge
20h 47m
Merged PRs (30d)
89

Description

Raised by Codex on #705 (`catchup.go`, after `Observer.PageDelivered`) and, in
the same round, by Copilot as **six** separate suppressed comments. Six
sightings of one shape is the signal to name the class rather than patch the
instances, so this issue is the class.

### The class

Every `Observer` callback and the `SignalHandler` run on the consumer's
goroutine, and `Connector.Close` is documented callable from any of them —
`deliver` already re-checks `l.runCtx.Err()` per delivery for exactly that
reason. But `Close` is the **universal Closed edge**: it must end iteration
cleanly, with nothing durable moving and no error element after it. No callback
site other than `deliver` re-checks, so whatever the code does next runs past
that edge.

The sites, and what each does after the edge:

| Site | Callback | What happens after `Close` returned |
|---|---|---|
| `catchup.go` (walk, per page) | `Observer.PageDelivered` | advances the in-memory position and calls `saveCheckpoint` — and the built-in file store deliberately ignores a cancelled context, so the **durable** checkpoint advances |
| `catchup.go` (streaming) | `Observer.PageDelivered` | same, on the repair path |
| `loop.go` (dial) | `Observer.Connected` | starts the frame pump, i.e. a new `ReadFrame` after the edge |
| `loop.go` (disconnect dispatch) | `Observer.Disconnected` | classifies the reason and can return a terminal outcome, yielding an error element instead of a clean stop |
| `loop.go` (overflow) | `SignalHandler` / `Observer.BufferOverflow` | a `Terminate` disposition still produces `Terminal(buffer_overflow)` |
| `recovery.go` (`recoverGone`) | `Observer.Gap` / `SignalHandler` | returns `Terminal(feed_gap)` |

Two distinct harms, and they want different answers. The checkpoint one is a
**durability** violation: a save landing after `Close` returned is the one thing
the clean-stop path promises does not happen. The others are **surface**
violations: an error element where §23 requires the Closed edge.

### Why this is one decision, not six patches

The obvious remedy — an `if l.runCtx.Err() != nil` after each callback — is six
edits today and a seventh the next time a callback is added, with nothing
enforcing it. That is the accretion shape, and the coverage would be exactly the
call sites someone remembered.

Better shapes to weigh:

- **Funnel the callbacks.** One `l.observe(func())` wrapper that invokes the
callback and reports whether cancellation landed during it, so the check is
structural rather than remembered. Costs a mechanical rewrite of every
callback site; buys the invariant for sites not written yet.
- **Check where the consequence is, not where the callback is.** Only two
things must not happen after the edge — a durable save, and a terminal
element. Guarding `saveCheckpoint` and the terminal construction covers every
site including future ones, with two edits instead of six. Does not cover the
pump start.
- **Decide it is out of scope and say so.** A callback that closes the
connector and then complains the connector did one more thing is arguably
asking for a stronger guarantee than §23 gives. If so, §23 should say which
operations the Closed edge preempts, and the `deliver` check should be
documented as the guarantee's boundary rather than read as a promise the
other sites break.

The third is a real candidate and would make the other two unnecessary; it
needs a §23 statement either way, because "callbacks may call Close" is a
published affordance and what it preempts is currently undefined.

### Scope

- Decide the rule in SPEC §23 (Consumer Surface / the Closed edge).
- Apply it to the Go reference connector, and to the other five SDKs' connectors
as they land.
- Whichever shape wins, the durable-save case wants a regression test: it is the
only one of the six with a consequence that outlives the process.

Follows #606, #614, #645, #696; sibling of #753 and #758.

Contributor guide

Open the contributing guide

Research direction

Start with SPEC §23 and trace the listed callback sites in catchup.go, loop.go, and recovery.go, including the existing deliver cancellation check. Decide which Closed-edge rule applies, then assess its effect across the Go connector and the other SDK connectors as they land. Done means the rule is specified, the chosen shape is implemented, and the durable checkpoint case has a regression test.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.