basecamp / basecamp/basecamp-sdk

Event feed: an occupied deferral slot skips the drain's protocol-fatal scan entirely

Open
#760 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`, `drainScan`'s deferred-slot early
return). **The finding is correct.** It is filed rather than patched because it
is the *fourth* correction to the same guarantee, and at that point the bound is
not what needs fixing.

### The finding

`drainScan` short-circuits whenever the deferral slot is occupied:

```go
if d := l.deferred; d != nil {
if !d.closed {
if f, ok := protocolFatalFrame(d.item); ok { ...terminate... }
}
return cycleOutcome{}, false // <- never looks at the queue
}
```

So when the slot holds a **non-fatal** frame — a recoverable disconnect, an
invalid frame — no draining scan ever examines the queue, for the whole drain.
An `invalid_event_stream_command` the pump had already queued behind it is
never seen. The drain completes, the held entry position saves, `caught_up`
announces, and dispatching the non-fatal deferral then disposes the attempt and
discards the server's protocol-fatal verdict. The connector reconnects where
§23 requires it to terminate with zero reconnects.

The code's own comment is the assumption that fails: *"Everything the pump has
queued arrived behind this one, so the scan stops here either way."* True about
arrival order, but the carve-out is not about arrival order — it is about which
verdict governs. §23 says only *recoverable* failures defer during Draining.

### Why this is an issue and not a patch

The ledger on this one guarantee, in order:

1. Scan bound sized by `liveBufferCapacity` → a fatal frame could hide behind a single ping under `WithLiveBufferCapacity(1)`. Rebounded on the pump queue's depth.
2. Bound = `pumpDepth` → missed the frame the pump had *read* but was blocked handing off. Corrected to `pumpDepth+1` (#705, `33719c8b1`).
3. This one: the bound is irrelevant, because an occupied slot means the scan never runs at all.

Three different escapes, one guarantee, and each fix was locally principled.
What they share is not the bound — it is **the single-slot deferral sitting in
front of the scan**. The slot has to hold the frame (order must be preserved),
which means the scan cannot continue past it without somewhere to put the next
non-handled frame it finds. Widening the bound a fourth time does not touch
that.

### What the fix has to decide

Scanning past an occupied slot needs an answer to: *what happens to the second
non-handled frame the scan encounters?* Candidates, none free:

- **Scan for a fatal only, discarding nothing.** Look ahead for
`invalid_event_stream_command` and terminate on it, leaving the non-fatal
deferral undispatched (the terminal disposal discards it anyway). Cheapest,
and defensible — §23 says the fatal governs. But "look ahead without
consuming" is not something a Go channel offers, so it means consuming
frames the drain then has nowhere to put.
- **A deferral queue instead of a slot.** Order preserved, scan unblocked,
every escape in the ledger closed at once. Costs a bounded queue and a
re-reading of "the single deferral slot" that §23's deferred-consumption
language currently implies.
- **Move the carve-out out of the drain.** Have the pump classify
`invalid_event_stream_command` at receipt and latch the verdict, so the drain
reads a flag rather than scanning a queue. Dissolves the whole class, changes
where protocol dispatch lives, and is the largest change.

The second and third are §23 questions binding all six SDKs, not Go-local
choices — which is why this needs a decision before another patch.

### Related

The same single-slot deferral is the obstacle in #758 (a staleness expiry
during an in-flight poll has no slot form and no wake). These two should
probably be decided together: a deferral *queue* with a stale-expiry form would
close both, and neither closes cleanly alone.

Follows #606, #614, #645, #696; siblings #753, #758, #759.

Contributor guide

Open the contributing guide

Research direction

Start in catchup.go at drainScan and read the linked history in #705, #758, and the related §23 discussion. Decide how Draining should preserve order while detecting a queued protocol-fatal frame, then define the cross-SDK behavior before choosing among a fatal-only scan, a deferral queue, or pump-side classification. Done means the fatal verdict is not discarded when a non-fatal deferral occupies the slot.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.