Skip to content

fix(server): avoid mutating the cached channels slice on includeEncrypted - #98

Merged
dborup merged 1 commit into
masterfrom
codex/fix-channels-cache-append
Sep 26, 2026
Merged

dborup merged 1 commit into
masterfrom
codex/fix-channels-cache-append

Conversation

@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Summary

handleChannels (cmd/server/routes.go) appended the encrypted-channels slice
directly onto the result of db.GetChannels() / store.GetChannels() when
?includeEncrypted=true. Both GetChannels implementations return a
mutex-guarded, TTL-cached slice directly to every caller with no defensive
copy. When that cached slice still had spare capacity (cap > len), append
wrote the encrypted rows into the spare capacity in place, mutating the
shared backing array that other concurrent callers — and the cache itself —
also hold a reference to. That is a real data race between concurrent
includeEncrypted=true requests, detectable with go test -race, and silent
memory corruption of the cache's unused capacity.

Fix

Both append call sites in handleChannels (the s.db branch and the
s.store branch) now use a full slice expression:

channels = append(channels[:len(channels):len(channels)], encrypted...)

This forces append to always allocate a fresh backing array instead of
writing into the cache's spare capacity. GetChannels itself, its caching
semantics, TTL, and locking are unchanged.

Tests

cmd/server/channels_cache_append_test.go drives GET /api/channels through
the real HTTP handler (both the s.db and s.store branches), seeds an
encrypted channel so includeEncrypted=true actually reaches the buggy
append, probes for a cached slice with spare capacity (bounded retries with
padding rows), and asserts the cache's full backing array is untouched after
concurrent includeEncrypted=true requests. A companion subtest fires many
concurrent includeEncrypted=true requests against a pre-warmed cache and
relies on go test -race to catch the race directly.

  • Red (unfixed code, go test -race -run TestChannelsCacheAppend -v ./...):
    the corruption assertion failed with an explicit before/after diff of the
    cache's spare-capacity slot, and -race reported WARNING: DATA RACE at
    the append call inside handleChannels for both the DB and store paths.
  • Green: after the fix, both subtests pass under -race, 3 consecutive
    runs, plus the full cmd/server suite under -race.
  • Mutation: reverting only the DB-path fix (leaving the store-path fixed)
    turns the DB-path test red while the store-path test stays green, and vice
    versa — proving the test exercises both call sites independently.

Note

This same fix is also present inside the (separate, larger) shared-channels
feature branch, since it was originally found and fixed there as part of that
work before being extracted into this standalone branch. This PR should be
merged first
, since the feature branch's history already contains this
exact fix as part of one of its commits.

… append

handleChannels (cmd/server/routes.go) appended the encrypted-channels
slice onto the result of db.GetChannels()/store.GetChannels() when
?includeEncrypted=true. Both GetChannels implementations return a
mutex-guarded, TTL-cached slice directly to every caller with no
defensive copy. When that cached slice still had spare capacity
(cap > len), append wrote the encrypted rows into the spare capacity
IN PLACE, mutating the shared backing array that other concurrent
callers — and the cache itself — also hold a reference to. That is a
real data race between concurrent includeEncrypted=true requests,
detectable with `go test -race`, and silent memory corruption of the
cache's unused capacity.

Fix both append call sites to use a full slice expression
(channels[:len(channels):len(channels)]) so append always allocates a
fresh backing array instead of writing into the cache's spare
capacity. GetChannels itself, its caching semantics, TTL, and locking
are unchanged.

Added cmd/server/channels_cache_append_test.go, which drives GET
/api/channels through the real HTTP handler (both the s.db and
s.store branches), seeds an encrypted channel so includeEncrypted=true
actually reaches the buggy append, probes for a cached slice with
spare capacity (bounded retries with padding rows), and asserts the
cache's full backing array is untouched after concurrent
includeEncrypted=true requests. A companion subtest fires many
concurrent includeEncrypted=true requests against a pre-warmed cache
and relies on `go test -race` to catch the race directly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

2 participants