fix: bound component shutdown against unreachable peers (SYN-110)

[cheggaaa]

Fixes SYN-110, plus the other instances of the same failure mode found by auditing the repo for it.

The bug

deleteLoop.Close did deleteCancel(); <-loopDone, but deleteFunc's call graph dropped the ctx twice — MarkTreeDeleted(context.Background(), …) and the settings object's AddContent(context.Background(), …). The latter reaches syncClient.Broadcast → peerManager.BroadcastMessage, which dials responsible peers and never returns against blackholed nodes. Cancelling did nothing, Close blocked forever, and the app.Close watchdog panicked with app.Close timeout.

Broadcast failures there are only logged, so honouring the ctx cannot fail a delete that is already committed locally.

Other instances

• concurrentLimiter.Close waited on its WaitGroup with no bound while the scheduled goroutine was inside a peer sync; its only ctx check happens before the action starts.
• syncAcl.broadcast ran under context.Background() while holding the acl lock, which syncAcl.Close then waits for. It also gates space.Description, SyncWithPeer and every aclSpaceClient call.
• actionPool built a cancel func and never called it, so every queued action ran under an effectively uncancellable ctx — also the ctx a syncTree holds its lock under, which is what made settings.Close unrecoverable.
• ocache.Close waited with context.Background() on entries the gc was already closing; that gc sits in peer.TryClose → drpc conn Close → a yamux stream whose peer never sends FIN, which yamux bounds at 5 minutes — past the app close deadline.
• peer.gc closed those conns while holding p.mu, freezing AcquireDrpcConn for the same peer.
• Missing timeouts: yamux never set StreamCloseTimeout (library default 5 min), and the webtransport dial was unbounded — DialTimeoutSec applied only to the accept path.
• Close before Run: app.Start's closeServices runs during the Init loop, so components get closed having never run. That deadlocked subscribeClient.Close on a channel only streamWatcher closes, stalled deleteLoop.Close for its full timeout, and nil-dereferenced actionPool.periodicLoop and streamPool.dial.

Approach

Cancellable contexts where the call graph is ours; a timeout plus the close ctx where it crosses into client-implemented interfaces (treemanager, peermanager) that may ignore cancellation. Shutdown may now leave work unfinished, which is the intent: the state is persisted and retried on the next run.

Because a bounded Close returns while the abandoned work is still running, concurrentLimiter rejects requests once closed — SyncWithPeer stays callable after close, and an Add landing on a WaitGroup that still has a parked waiter panics with WaitGroup is reused before previous Wait has returned.

ocache is deliberately conservative. The GO-7332 invariant — exactly one remover owns and closes a given close channel — holds: the ctx escape only removes a waiter and never takes ownership, and remove returns before touching e.value.Close() or setClosed. The behaviour change is confined to Close(); every other caller passes its own ctx and is unchanged. The deadline is spent only on entries another closer holds: loads are cancelled before the pass and value.Close() takes no ctx, so uncontended entries still close normally once it has expired, and total close time still scales with cache size.

Testing

New regression tests for each bounded close and each close-before-run path, including the two failure modes above — each fails against the code it replaces. Full suite green, -race green across commonspace, util, app, coordinator, acl, net.

Pre-existing and untouched: net/transport/webtransport fails under -race on a clean v0.13.x too — the race is inside wt.Server between Close() and Serve().

Not addressed

• The per-component budgets are additive. Each space gets 10s (deletionmanager) + 10s (keyvalue) of hard timer, and commonspace/space.go:250 passes context.Background(), so every ctx.Done() arm here is dead in production and only the timers apply. Three spaces closing serially reach app.StopDeadline (60s). The fix is a single space-level budget at space.go:250 with these timers as backstops, which changes shutdown semantics for all ~15 space components.
• syncAcl.Close still takes an unbounded s.Lock(). Cancelling the broadcast ctx unblocks it only if the client's peerManager honours cancellation.
• settingsObject.addContent widens an existing torn-write window. AddContentWithValidator advances the in-memory tree before persisting, so a cancelled client ctx can leave a head that was never written. Any AddAll failure already does this; ctx cancellation is a new trigger.
• Timeouts are hardcoded where quic, webtransport and ocache itself expose config or options for the equivalent knobs, and yamux's StreamCloseTimeout borrows WriteTimeoutSec rather than having its own field.

🤖 Generated with Claude Code

[cheggaaa]

Review

Reviewed with two independent passes plus verification of every claim against a running repro. The direction is right, but the first version had two regressions that were worse than the hang it fixed. Both are now fixed in a7ee564 with tests that fail against the previous code.

Fixed during review

1. concurrentLimiter.Close could panic the process. When Close returned on timeout, its wg.Wait() helper stayed parked. The abandoned request eventually calls wg.Done(), waking that waiter — and any wg.Add(1) landing in between panics with sync: WaitGroup is reused before previous Wait has returned. ScheduleRequest had no closed flag, and SyncWithPeer is explicitly documented as safe to call after close (headsync.go:114), so the Add is reachable. Reproduced:

panic: sync: WaitGroup is reused before previous Wait has returned
	.../keyvalue/limiter.go:63
created by (*concurrentLimiter).Close

Fixed by rejecting requests once closed.

2. ocache.Close left roughly half the healthy entries unclosed. The shared deadline was passed to remove, which calls waitLoad first. For a loaded entry both <-ctx.Done() and <-e.load are ready, and Go's select picks at random — so once the budget expired, every remaining entry had a coin-flip chance of returning ctx.Err() before value.Close() was ever called, and was left in c.data with c.closed already true, where nothing can ever close it. Measured 91–116 of 200 entries abandoned. In this repo that is net/pool's caches: one stuck peer burns the budget, then healthy peers keep their conns and acceptLoop goroutines after pool.Close(). Nondeterministic per run, so it would have surfaced as an intermittent field bug.

Fixed by spending the deadline only on the wait-for-another-closer, with loads on an uncancellable ctx (Close already cancels them just above). The comment claiming the deadline bounds the whole pass was wrong and has been corrected — value.Close() takes no ctx, so total close time still scales with cache size.

3. deleteLoop.Close burned the full 10s when Run never ran — the same close-before-run case the PR guards for subscribeClient, actionPool and streamPool. Guarded now.

Confirmed sound

• webtransport defer cancel() does not kill the returned session: HandshakeOutbound builds cctx from context.Background() (secureservice.go:171), and webtransport-go re-parents the session with context.WithoutCancel. Same pattern yamux already uses.
• GO-7332 / ba23586 invariants hold. The ctx escape in setClosing only drops a waiter — it never assigns e.close or claims ownership, and remove returns before value.Close()/setClosed. No double close of a channel or a value.
• peer.gc deferred close is correctly ordered (LIFO puts it after Unlock), and conns leave active/inactive under the lock, so no double close.
• No API breakage: everything changed is unexported, and the OCache interface and all Close(ctx) signatures are unchanged.

Open, deliberately not addressed here

1. The per-component budgets are additive and can still blow the watchdog. Each space now gets 10s (deletionmanager) + 10s (keyvalue) of hard timer, and commonspace/space.go:250 passes context.Background(), so every ctx.Done() arm added here is dead in production — only the timers apply. Three spaces closing serially reaches app.StopDeadline (60s) on its own. The real fix is a single space-level budget at space.go:250 with the per-component timers as backstops, and constants meaningfully below it. That changes shutdown semantics for all ~15 space components, so it wants its own change.
2. syncAcl.Close still takes an unbounded s.Lock(). Cancelling broadcastCtx unblocks it only if the client's peerManager honours ctx — and peermanager is client-implemented, the same category as treemanager, which this PR deliberately does not trust.
3. settingsObject.addContent now widens an existing torn-write window. AddContentWithValidator advances the in-memory tree (objecttree.go:294) before persisting (:299), so a cancelled client ctx can leave a head that was never written. Any AddAll failure already does this, so it is pre-existing rather than new, but ctx cancellation is a new trigger on a client-facing path.
4. Timeouts are hardcoded where this repo's convention is config. quic and webtransport both expose CloseTimeoutSec; ocache already has WithTTL/WithGCPeriod options. All three new constants are exactly app.StopWarningAfter (10s), so any one firing guarantees the "components close in progress" warning.
5. yamux StreamCloseTimeout reuses WriteTimeoutSec. Semantically correct — it RSTs only the stream, not the session — but it is a different quantity, and lowering the write timeout would silently start resetting merely-slow streams. Deserves its own config field.
6. Untested behaviour: the syncacl broadcast ctx (no Close test exists in that package at all), the deleter ctx.Err() bail-outs, the settings ctx plumbing, peer.gc's post-unlock close, and both transport timeouts.
7. Downstream risk this strategy assumes: any-store's ConnManager.Close closes read conns without waiting for checked-out ones, so a goroutine still executing a statement hits a closed sqlite handle. Before this PR no component goroutine outlived its Close; now they can. Not fixable inside any-sync, but the embedder should know.

Verdict

Mergeable once the reviewer agrees on (1) — the additive budgets are the difference between "bounded" and "bounded per component but not overall". Everything else above is follow-up.