Browse Source

fix(host,client): roll back partial flow registrations; cover the widened double

- The browse client half mirrors -native's rollback: a pair construction
  that throws halfway (declared-but-occupied second hole) disposes the
  earlier deferral before rethrowing, so no orphaned slot subscription
  outlives the failed fiber (ds-review-bot).
- The TestWorkspaces browse methods gain the coverage CI's per-file gate
  requires (defaults + stub overrides, recorded like their siblings).
creatixchu 2 months ago
parent
commit
761ceb79f3

+ 20 - 0
packages/client/test-runtime/tests/runtime.spec.tsx

@@ -322,6 +322,26 @@ describe('workspaces', () => {
     expect(stub).toHaveBeenCalledOnce()
     await runtime.dispose()
   })
+
+  it('records the browse calls: listDirectory serves an empty home, createDirectory joins, stubs override', async () => {
+    const runtime = await runtimeWithFrame()
+    // Defaults: an empty home level and parent/name joining.
+    await expect(runtime.workspaces.listDirectory()).resolves.toMatchObject({ path: '/home/test', entries: [] })
+    await expect(runtime.workspaces.listDirectory('/home/test')).resolves.toMatchObject({ path: '/home/test' })
+    await expect(runtime.workspaces.createDirectory('/home/test', 'fresh')).resolves.toBe('/home/test/fresh')
+    expect(runtime.workspaces.calls).toEqual([
+      { method: 'listDirectory', args: [undefined] },
+      { method: 'listDirectory', args: ['/home/test'] },
+      { method: 'createDirectory', args: ['/home/test', 'fresh'] },
+    ])
+    // Stubs replace the defaults like every sibling method.
+    const listing = { path: '/x', home: '/x', crumbs: [], entries: [] }
+    runtime.workspaces.stub('listDirectory', vi.fn(() => Promise.resolve(listing as never)))
+    runtime.workspaces.stub('createDirectory', vi.fn(() => Promise.resolve('/x/made' as never)))
+    await expect(runtime.workspaces.listDirectory('/x')).resolves.toBe(listing)
+    await expect(runtime.workspaces.createDirectory('/x', 'made')).resolves.toBe('/x/made')
+    await runtime.dispose()
+  })
 })
 
 describe('feature mount and disposal', () => {

+ 13 - 6
packages/host/directory-picker-browse/src/client/index.ts

@@ -98,12 +98,19 @@ export function apply(ctx: ClientContext): void {
     t: ctx.locale.bind(LOCALE_NS),
   })
   ctx.effect(() => {
-    const deferred = [
-      deferRegistration(ctx.slots, 'conversation.hero.workspace.directoryFlow', BrowseDirectoryFlow, () =>
-        ctx.slots.register({ name: 'conversation.hero.workspace.directoryFlow', inject: injected }, BrowseDirectoryFlow)),
-      deferRegistration(ctx.slots, 'sidebar.workspaces.directoryFlow', BrowseDirectoryFlow, () =>
-        ctx.slots.register({ name: 'sidebar.workspaces.directoryFlow', inject: injected }, BrowseDirectoryFlow)),
-    ]
+    // Constructing the pair can throw halfway (a declared hole already
+    // occupied registers synchronously): roll the earlier deferral back so
+    // no live subscription outlives the failed fiber.
+    const deferred: ReturnType<typeof deferRegistration>[] = []
+    try {
+      deferred.push(deferRegistration(ctx.slots, 'conversation.hero.workspace.directoryFlow', BrowseDirectoryFlow, () =>
+        ctx.slots.register({ name: 'conversation.hero.workspace.directoryFlow', inject: injected }, BrowseDirectoryFlow)))
+      deferred.push(deferRegistration(ctx.slots, 'sidebar.workspaces.directoryFlow', BrowseDirectoryFlow, () =>
+        ctx.slots.register({ name: 'sidebar.workspaces.directoryFlow', inject: injected }, BrowseDirectoryFlow)))
+    } catch (error) {
+      for (const entry of deferred) entry.dispose()
+      throw error
+    }
     return () => { for (const entry of deferred) entry.dispose() }
   }, 'directory-picker-browse: flow registrations')
 }

+ 25 - 0
packages/host/directory-picker-browse/tests/client-flow.spec.tsx

@@ -66,6 +66,31 @@ describe('directory-picker-browse client half', () => {
     for (const hole of HOLES) expect(after.slots.entries(hole)).toHaveLength(1)
   })
 
+  it('rolls back the first deferral when the second hole is already occupied', async () => {
+    const b = await bench()
+    b.declare()
+    // Foreign occupant in the SECOND registered hole: the pair construction
+    // throws after the first deferral installed its subscription.
+    b.slots.register({ name: HOLES[1] } as never, () => null)
+    const rejections: unknown[] = []
+    const onUnhandled = (reason: unknown): void => { rejections.push(reason) }
+    process.on('unhandledRejection', onUnhandled)
+    try {
+      const fiber = b.ctx.plugin({ inject: [...inject], apply })
+      await expect(fiber.await()).rejects.toThrow(/already has a registration/)
+      // A leaked first deferral would now race this probe registration and
+      // throw from its orphaned subscription against the HERO hole; the
+      // rollback leaves only the activation failure itself (cordis re-raises
+      // the apply throw as a late rejection — installFailLoud's contract).
+      const disposeProbe = b.slots.register({ name: HOLES[0] } as never, () => null)
+      await new Promise(resolve => setTimeout(resolve, 20))
+      expect(rejections.map(String).filter(text => text.includes(HOLES[0]))).toEqual([])
+      disposeProbe()
+    } finally {
+      process.off('unhandledRejection', onUnhandled)
+    }
+  })
+
   it('registers the dialog dictionaries and binds this package namespace', async () => {
     const b = await bench()
     b.declare()