fix: drop cached project instance on rejoin - #199
Conversation
Regression test for digidem/comapeo-mobile#2042 and #2041: accepting a re-invite after leaving a project closes the stale project instance on the manager, and with @comapeo/ipc 9.0.0 the client kept returning the closed instance, so every project call rejected with ProjectClosed until app restart. Fixed by @comapeo/ipc 9.0.1, which this test verifies end-to-end through the hooks (accept invite, use project, get removed, leave, re-accept, use project again). Runs in the node environment because the jsdom environment replaces the Uint8Array global, which breaks @comapeo/core's timing-safe inviteId comparison (cross-realm instanceof) and aborts the invite flow.
Accepting an invite (re-)adds the project on the backend (MapeoManager.addProject), which closes any project instance that was open before the invite — e.g. after being removed from and leaving the project — and opens a fresh one. useSingleProject caches the project client with staleTime/gcTime Infinity, so every hook kept using the closed instance and rejected with ProjectClosed until app restart. Fixes digidem/comapeo-mobile#2042 and digidem/comapeo-mobile#2041 together with @comapeo/ipc >= 9.0.1, which re-validates the cached instance against the server on getProject.
|
| Name | Status | Previous | Current |
|---|---|---|---|
@comapeo/ipc |
9.0.0 | 9.0.1 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
achou11
left a comment
achou11
left a comment
There was a problem hiding this comment.
Change seems solid based on added regression test. Would still like some clarification on the question I have about the cause of the issue though.
| // Accepting an invite (re-)adds the project on the backend, which | ||
| // closes any project instance that was open before the invite (e.g. | ||
| // after leaving the project) and opens a fresh one. The project | ||
| // client is cached with staleTime/gcTime Infinity, so drop it here | ||
| // or every observer keeps using the closed instance. | ||
| queryClient.removeQueries({ | ||
| queryKey: getProjectByIdQueryKey({ projectId }), | ||
| exact: true, | ||
| }) |
There was a problem hiding this comment.
To clarify, is the issue more specific to having gcTime: Inifinity? From my understanding of the docs here, the existing invalidation of all project queries just below should already account for queries using staleTime: Infinity due to inexact query matching:
set staleTime to Infinity to never trigger a refetch until the Query is invalidated manually.
'static' and Infinity both prevent staleness-based refetches, but 'static' is stricter: queryClient.invalidateQueries() can invalidate a query with staleTime: Infinity, but has no effect on staleTime: 'static'. refetchOnMount, refetchOnWindowFocus, and refetchOnReconnect set to "always" are also blocked by 'static'. Use 'static' for data that cannot change while the app is running: feature flags fetched at boot, user permissions loaded at login, static reference tables. Use Infinity when you still want manual invalidation to work.
Or is this tangential to what you're describing?
There was a problem hiding this comment.
Ah this detail about the difference between removeQueries and invalidateQueries maybe helps a bit:
Unlike invalidateQueries or refetchQueries, removeQueries removes matching queries from the cache instead of refetching them.
So I guess the existing invalidation is triggering a refetch of the project instance, but maybe the refetch is returning a value that is referentially equivalent to the stale instance, and thus the observers are still using the stale one? Can't fully remember how IPC handles getProject calls for existing project instances.
Could be way off here...
There was a problem hiding this comment.
I think the issue is that regardless of the gcTime and staleTime, react-query will serve the "stale" reference first, while it refetches the new reference, and that's what causes our errors. We could stop caching anything in react-query, but that would increase IPC traffic because getProject() does a round-trip check that the project is not closed for every call. However I think this is all getting rather messy to I'm considering dropping the changes to comapeo-ipc which made it "instance aware" at the expense of not being able to simply run the core e2e tests in core-react-native (the e2e tests as written require full lifecycle open -> close control)
| // (`MapeoManager.addProject`) and opens a fresh one. The project client | ||
| // wrapper is cached with `staleTime: Infinity`, so without invalidation the | ||
| // hooks keep using the closed instance and every project call rejects with | ||
| // ProjectClosed until app restart. |
There was a problem hiding this comment.
Similar question about precision of the root cause of the issue. My understanding is that it's more about the gcTime setting than the staleTime setting.
achou11
left a comment
achou11
left a comment
There was a problem hiding this comment.
One small thing that I found while re-reviewing
## Optic Release Automation This **draft** PR is opened by Github action [optic-release-automation-action](https://github.com/nearform-actions/optic-release-automation-action). A new **draft** GitHub release [v12.0.3](https://github.com/digidem/comapeo-core-react/releases/tag/untagged-39a2ab34637dfd20e41d) has been created. Release author: @RangerMauve #### If you want to go ahead with the release, please merge this PR. When you merge: - The GitHub release will be published - The npm package with tag latest will be published according to the publishing rules you have configured - No major or minor tags will be updated as configured #### If you close the PR - The new draft release will be deleted and nothing will change ## What's Changed * fix: drop cached project instance on rejoin by @gmaclennan in #199 **Full Changelog**: v12.0.2...v12.0.3 <!-- <release-meta>{"id":366207480,"version":"v12.0.3","npmTag":"latest","opticUrl":"https://optic-zf3votdk5a-ew.a.run.app/api/generate/"}</release-meta> --> Co-authored-by: RangerMauve <actions@users.noreply.github.com>
Dropping the identity assertion left the regression without anything to fail on. Assert instead what #199 was actually about: a call made directly through the reference the hooks hand out after re-joining reaches a live instance rather than the closed one that was cached before the leave. Asserted through the reference rather than only through the settings query, and deliberately not through the pre-leave reference - that one recovers under v10, where references are permanent, but stays closed under v9, so it is not a property both majors share.
3 participants
useSingleProjectcaches the project client withstaleTime/gcTimeInfinity, but in @comapeo/ipc >=9 project instances have a lifecycle and can be closed. Without this fix, re-joining a project re-used a cached closed project instnace resulting in aProjectClosederror.This fix drops the cached project client in
useAcceptInvite'sonSuccess(removeQuerieson the project-by-id key), so the next observer fetches a fresh instance.@comapeo/ipc>= 9.0.1 then validates the instance against the server ongetProject, returning a working client for the re-opened project. The ipc devDependency is bumped to 9.0.1 accordingly.Includes a regression test that runs two real
MapeoManagers connected as local peers, driving the invitee through the hooks over real IPC: accept invite → use project → get removed → leave → re-accept → use project. It fails withProjectClosedwithout the fix and passes with it. The test runs in the node environment because the jsdom environment replaces theUint8Arrayglobal, which breaks@comapeo/core's timing-safe inviteId comparison and aborts the invite flow.Verified end-to-end locally with this fix applied: the remove → leave → re-invite → accept cycle that previously crashed with
ProjectClosednow completes and the re-joined project is fully usable.