Skip to content

ocache: refuse to close an entry that is still loading (GO-7333) - #769

Open
requilence wants to merge 1 commit into
mainfrom
go-7333-ocache-tryremove-loading
Open

ocache: refuse to close an entry that is still loading (GO-7333)#769
requilence wants to merge 1 commit into
mainfrom
go-7333-ocache-tryremove-loading

Conversation

@requilence

Copy link
Copy Markdown
Contributor

Refs anytype-heart GO-7333. Follow-up to #746 (GO-7332), found while reviewing it.

Problem

TryRemove reaches e.value.TryClose(c.ttl) on an entry that is still in entryStateLoading, where e.value is nil:

// app/ocache/ocache.go:319-321 (before)
prevState, _, _ := e.setClosing(context.Background(), false)
if prevState == entryStateClosing || prevState == entryStateClosed {
	return false, nil
}
closed, err := e.value.TryClose(c.ttl)

entryStateLoading is the zero value of the state (entry.go:14) and nothing bailed on it. An entry is loading from newEntry(id, nil, entryStateLoading) (ocache.go:150) until oCache.load publishes the value (ocache.go:227), so the window is every in-flight load. Three failure modes:

  1. Nil dereference. e.value is a genuinely nil Object, so the method call panics — and it panics in the caller's goroutine, taking the process with it.
  2. Permanent hang. setClosing flips the entry loading→closing and creates a fresh e.close, but e.load is closed only by defer close(e.load) inside oCache.load, and the load's success path calls setActive(false), which does not close e.close. Any later Get (waitClose first) parks on that channel forever.
  3. Data race on e.value. oCache.load writes it under c.mu (ocache.go:227); TryRemove read it after releasing c.mu, holding neither c.mu nor e.mx. -race on the new test reports exactly this pair before the fix.

TryRemove has no production callers inside any-sync (tests only), but anytype-heart calls it on a high-volume path — core/indexer/fulltext.go TryRemoveFromCache after every fulltext index of an object, plus core/block/editor/layout/syncer.go — so the exposure is real, gated only on the object still being loaded elsewhere.

Fix

setClosing refuses the transition for a loading entry instead of performing it, and the two try-close paths (TryRemove, GC) act only when prevState == entryStateActive — the state in which the call actually acquired the transition.

Why the transition must be prevented, not undone or skipped. Adding || prevState == entryStateLoading to the old bail condition does not fix the bug, it converts failure mode 1 into failure mode 2: by the time the caller sees prevState, setClosing has already run e.state = entryStateClosing; e.close = make(chan struct{}), so the entry stays parked in closing with an e.load that is still open and an e.close nobody will close. Restoring with setActive(true) is worse: it marks the entry active while e.value is still nil, so the very next GC pass — which pre-filters on e.isActive() — walks straight into the same nil dereference. Both variants were implemented and run against the new test: the naive bail-out fails on the stranded-Get subtest (and leaves Close eating its full timeout), and the setActive(true) variant fails with GC panicked on a loading entry: invalid memory address or nil pointer dereference.

The other two setClosing callers

  • removeCtx (ocache.go:268, wait=true; used by Remove, RemoveSame, Close) calls e.waitLoad first, and <-e.load is closed only after the load has either set the value and called setActive or recorded loadErr and deleted the entry — so it never arrives here in the loading state. If it somehow did, it now gets curState != entryStateClosing(false, nil), which is a safe refusal rather than a nil Close(). Close still cancels every in-flight load (e.cancelLoad()) and then closes the value the load produces; two new subtests pin that, one where the loadFunc ignores the cancellation and yields a value (it must be closed) and one where it honours it (Close must not hang).
  • GC (ocache.go:413) builds toClose from e.isActive() entries. The window between that check and setClosing cannot expose a loading entry: entryStateLoading is written in exactly one place, newEntry (ocache.go:150), and a removed entry is replaced by a fresh one rather than reset, so there is no active→loading edge. The prevState != entryStateActive change there is defensive and behaviourally identical for closing/closed; it is what keeps the setActive(true) trap above from being reachable through GC.

The GO-7332 for e.state == entryStateClosing loop is untouched — the new guard sits above it, and the loading state never enters that loop anyway (the loop's exit states are active/closed, never loading).

Test

TestOCache_TryRemoveWhileLoading (app/ocache/ocache_test.go), deterministic — the loadFunc signals when it is in flight and is released by the test, in the style of the existing load-synchronising tests:

  • refuses a loading entry and leaves the load running — mode 1: TryRemove returns (false, nil) without panicking, the entry is still entryStateLoading afterwards, and the in-flight Get completes with its value.
  • a Get arriving after the refused TryRemove is not stranded — mode 2: a Get issued after TryRemove must not park forever in waitClose.
  • GC never closes a loading entry — GC before and after a refused TryRemove, catching the setActive(true) restore variant.
  • Close still closes an entry that was loading / Close does not hang on a load it aborted — the shared-behaviour change to setClosing does not regress cache shutdown.
  • TryRemove racing the value publication — mode 3: the load publishes e.value while TryRemove reads it, so -race observes the pair (100 iterations).

Verification:

$ go test ./app/ocache/... -race -count=1
ok  	github.com/anyproto/any-sync/app/ocache	3.522s
$ go test ./app/ocache/... -race -count=10
ok  	github.com/anyproto/any-sync/app/ocache	59.218s
$ go test ./acl/... ./commonspace/... ./net/... -race -count=1   # every package importing ocache
ok  (35 packages, no failures)

TryRemove marked a loading entry closing and then called TryClose on its
value - which is nil until oCache.load publishes it. Three failure modes:
a nil dereference; the entry left parked in closing behind a close channel
nobody ever closes, stranding every later Get/Remove; and an unsynchronised
read of e.value, written by load under c.mu.

setClosing now refuses the loading state instead of transitioning it, and
the two try-close paths act only on prevState == entryStateActive - the
state in which the call actually acquired the transition. Bailing out after
setClosing has run is not equivalent: the entry is already closing by then,
and setActive(true) would make it active with a nil value for the next GC
pass to dereference.

Remove/RemoveSame/Close wait the load out in removeCtx before they get
here, so they are unaffected: Close still cancels loads and closes the
values they produce.
@github-actions

Copy link
Copy Markdown

New Coverage 60.1% of statements
Patch Coverage 100.0% of changed statements (11/11)

Coverage provided by https://github.com/seriousben/go-patch-cover-action

@requilence
requilence requested a review from cheggaaa August 26, 2026 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant