fix(sync): a cascade-deleted child says which drive it left - #1315
Merged
Conversation
Destroying a folder removes its contents server-side, and the event that
announces each child carried `drive: None`. The fanout uses that to route:
let Some(owner) = msg.drive.as_ref() else { return; };
so the removal reached only subscribers of the child's own subject — and
in the v2 protocol there are none. `Store.subscribeWebSocket` is a
documented no-op; clients subscribe once, per drive. Every cascade-deleted
child was therefore announced to nobody.
What that costs: another tab keeps rendering resources that no longer
exist, and the tab that issued the destroy keeps them in its local
database, where a reload brings them back. The parent vanished and its
contents did not, which reads as a half-finished delete.
The drive has to be read where the resource still exists — a listener
reacting to the event cannot look it up, because by then it is gone. So
`DbEvent::Destroyed` carries it, and `recursive_remove` passes its own
drive down as the fallback for a child created before the server stamped
one.
The test asserts the invariant that actually matters: the removal names a
drive. It is built on a genesis commit with rights validation on, because
that is the path that stamps `drive` — a plain `save` skips the stamp, and
the test would then pass for a reason the app never meets.
`getCurrentSubject` was read straight after clicking "New Folder", but QuickCreateRow fires `createNewResource` without awaiting it, so the click returns before the navigation. The subject read there is the PARENT's. The test then destroyed that parent and asserted it was gone — which it was, directly, by the commit it had just posted. It never once exercised the cascade it exists to cover, and whether the navigation had landed depended on load, which is what made it look flaky rather than wrong. So wait for the URL to change, as `sidebar subresource` already does, and assert the two subjects differ — a test that silently checks the wrong resource should fail, not pass. With the child's real subject the barrier before the reload becomes meaningful: it waits for the cascade's removal to arrive over the drive subscription. Reverting the server fix now fails this test in 15s.
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.
Destroying a folder removes its contents server-side, and the event that announces each child carried
drive: None. The fanout routes on exactly that:So the removal reached only subscribers of the child's own subject — and in the v2 protocol there are none.
Store.subscribeWebSocketis a documented no-op; clients subscribe once, per drive. Every cascade-deleted child was announced to nobody.What that costs a user: delete a folder, and another open tab keeps rendering its contents. The tab that issued the destroy keeps them in its local database, where a reload brings them back. The parent vanishes and its children do not, which reads as a half-finished delete.
The fix
The drive has to be read where the resource still exists — a listener reacting to the event cannot look it up, because by then it is gone. So
DbEvent::Destroyedcarries it, andrecursive_removepasses its own drive down as the fallback for a child created before the server stamped one.Why the e2e never caught it
getCurrentSubjectwas read straight after clicking "New Folder", butQuickCreateRowfirescreateNewResourcewithout awaiting, so the click returns before the navigation. The subject read there is the parent's — confirmed directly with a probe:nestedandparentwere the same string.The test then destroyed that parent and asserted it was gone, which it was, by the commit it had just posted. It never once exercised the cascade it exists to cover. Whether the navigation had landed depended on load, which is what made it look flaky rather than wrong.
It now waits for the URL to change (as
sidebar subresourcealready does) and asserts the two subjects differ — a test that silently checks the wrong resource should fail, not pass.Verification
drive— a plainsaveskips the stamp and the test would pass for a reason the app never meets.left: None), passes with it.within=true subs=1. A client-side probe confirmed the other end: the child now appears inremoveResourcecalls and leaves the store.delete resourcee2e passes; reverting just the server fix fails it in 15s.e2e.spec.ts+tables.spec.tstogether — the pairing that used to reproduce the failure locally — 23 passed.The e2e runs were done on
feat/plugin-model, where this was originally found; the commits cherry-picked onto develop with no conflicts and the lib suite is green here.