Repository navigation
fix(social): drop a roster answer that lands after a newer one (#224) - #242
Merged
Merged
Conversation
…lter does not recount a hide (ThinkFlowLab#224)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
remoteBots()numbered roster fetches when their answer landed (rosterAt = ++rosterFetchesafterlist_remote_botsreturned).hideRemotestarts a backgroundremoteBots({ fresh: true })after leaving rooms, so on a loaded machine (or a slow link) an older fetch could ask while the Bot was not shared, land after a newer fetch that already saw the re-share, take a newer number and overwriteremote— the re-shared Bot disappeared again. The newer caller returns the sharedremote, which is exactly the intermittent#90: a hidden Bot shows again only after the owner re-shares itfailure (#224, seen locally and once on CI in #225's run).Second, smaller defect found on the way: the grant re-filter (
syncOutgoingGrants().then(...)) re-applies the same server answer throughapplyRemoteRoster, and that counted as another absent fetch toward forgetting a local hide (HIDE_GONE_FETCHES = 2), so one absent answer could forget a hide.What changed
rosterAsked); an answer whose number is below the last applied one (rosterApplied) is dropped before it touches the roster,declinedRemotes, the inbound backfill or the hide counter. The late grant re-filter guard now compares againstrosterApplied(same Accept invite: 'Talk through' copy implies a Bot is granted back; People with access stays stale; Helper appears ~10 s after the toast #228 intent).applyRemoteRoster(..., { refilter: true })for the grant re-filter: it does not callforgetHiddenIfGone.docs/subsystems/social.md(Per-Bot grants leak: accepting a Helper invite exposes the joiner's Chief; face-room Chief shows in Friends' Bots; Manage access wrong/raw #138 bullet); changelog fragment.Plugin-only, no
core/change, so no client build or patch regeneration.Tests
Two new deterministic tests in
plugins/social/test/social.test.js, both red at base (7a3ccb3, verified by stashingsrc/social.js) and green here:#224: a roster answer that lands after a newer one is dropped…— holds onelist_remote_botsreply (the server read the list when asked), lets a newer fetch see the re-share and land first, then releases the old one: at base the roster loses Coder again.#224: the grant re-filter of one answer does not count as a second absent fetch— fastlistGrants; at baseabsentFetchesbecomes 2 after one absent answer.The existing
#90test is unchanged and now cannot hit the race.Verification
TZ=UTC ARCH_DOCS_BASE=upstream/main npm run verifyon this tip (Node 24.11.1, Linux): exit 0 — lint +arch-docs: OK (4 changed files), build-client check, patches check, core 790/0, social 184/0, tree 56/0,.github/scripts78/0, interop and live-e2e lint+test pass.Not run: smoke test (no
core/change), real-model bench.Fixes #224