Observer verification: fix coverage bookkeeping, restart sessions when scope widens

[Maximo-Guk]

Why:

Before this PR: We didn't sever existing observers, this was just actually documented in the original plan "existing observers see an incremental modal for just the new binding on their next open..."

The main gap we have right now is adding a binding does not restart live sessions, so an already-open collaborator is only verified against it at their next open.

What:

authorizeCollaborator() is now responsible for being the single gate for checking whether observers have access to underyling data. Everything routes through here, including receiveExternalMessage() which previously only checked the role and never checked whether the observer had access to the underlying data.

We also decided to abort the DO in order to revoke the RPC capabilities, we already use the same mechanism when revoking collaborators and we can just the same mechanism here. We made this a little less disruptive by checking graph first to see if there's any other collaborators before aborting the DO

Testing:

I added extensive integration tests in observer-role-scope.test.ts, observer-reverification.test.ts and external-message-verification ( even though it's actually used yet in any gatekeepers ) for testing the observer verification logic

[github-actions]

Preview: pr380-observer-sess-c41973bd

https://pr380-observer-sess-c41973bd-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[ask-bonk]

[P1] Keep the widened capability hidden until old sessions are invalidated

The record becomes collaborator-visible here, but the restart below still has to resolve getSharingManager(), sync storage, and sleep for 100 ms. During that interval an existing build collaborator can use the sequential ID to pipeline getGatekeeperById(id).openSession() and read from this brand-new resource without ever passing observer verification. Unlike the revocation delay, this is a first-time grant of access, so the grace window violates the observer invariant. Please gate access by the session generation / verified scope until reconnect, or otherwise invalidate collaborator sessions before publishing the record.

[kentonv]

Note that, obviously, any writes have to happen before the abort or they will be lost. Moreover, the grace period is important to make sure writes actually settled. An abort() breaks the output gate, causing existing in-flight writes never to land. At the very least, we have to await ctx.storage.sync(), but that also gives a chance for a client to jump in with a request.

Perhaps the easiest solution is to use blockConcurrencyWhile().

await this.ctx.blockConcurrencyWhile(async () => {
  await this.ctx.storage.sync();

  // Throwing from blockConcurrencyWhile also resets the DO.
  throw new Error("Gadget restarted because a new connection was added.");
});

Another option would be to store in-memory that the given gatekeeper should be blocked from being accessed -- the mark naturally goes away after restarting. But, this seems more complicated, and blockConcurrencyWhile() should do the trick I think.

[ask-bonk]

[P1] Do not let this delayed abort overtake a later access mutation

A widening call can sync and enter the 100 ms wait, then a concurrent owner request can remove a collaborator: the permission edge is written synchronously, but that request awaits observer teardown/listing refresh before reaching its own scheduleAccessRestart(). The earlier timer can therefore call ctx.abort() first; because abort ignores the output gate, the later revocation write may be lost and the collaborator can become authorized again after restart. Coalesce these restarts and perform a final sync while preventing new access mutations immediately before aborting.

[ask-bonk]

[P1] Wait for the restart triggered by this denial

This failed re-verification scrubs Bob's persisted choice and schedules ctx.abort() about 100 ms later, but the test returns immediately and disposes its workspace/session. observer-reverification.test.ts documents that an abort landing after the last client leaves crashes local workerd; because these tests are concurrent, this can fail an unrelated sibling. Keep a probe alive and wait until it observes the disconnect (or otherwise settle the restart) before leaving the test.

[ask-bonk]

Submitted 3 actionable inline review findings.

github run

[ask-bonk]

[P1] Invalidate existing sessions before awaiting repair UI

By this point a failed addObserver() may already have scrubbed persisted coverage, but the restart is not scheduled until this catch runs. A configured open first awaits the collaborator-controlled configureCb.configure() retry (and later diagnostic/rollback RPCs), so a callback that never settles leaves other previously admitted sessions usable indefinitely after re-verification confirms the collaborator no longer has access. Revoke the old session generation, or at least schedule/coalesce the restart, immediately when the persisted choice is scrubbed and before any repair callback is awaited.

[ask-bonk]

[P2] Restart only when the affected role is present

This checks for any collaborator even though the callers widen different scopes: addGatekeeper() widens only build, while binding/promotion widens only use. Adding an unbound connection to a workspace with only use collaborators, or binding one when all collaborators are build, therefore aborts every in-flight session although nobody has new verification requirements. Pass the affected role into this helper and schedule only when listCollaborators() contains that role.

[ask-bonk]

Submitted 2 actionable inline review findings.

github run

[kentonv]

I started reviewing this but I see you're making other changes so decided to post what I have and come back later.

[kentonv]

Note that, obviously, any writes have to happen before the abort or they will be lost. Moreover, the grace period is important to make sure writes actually settled. An abort() breaks the output gate, causing existing in-flight writes never to land. At the very least, we have to await ctx.storage.sync(), but that also gives a chance for a client to jump in with a request.

Perhaps the easiest solution is to use blockConcurrencyWhile().

await this.ctx.blockConcurrencyWhile(async () => {
  await this.ctx.storage.sync();

  // Throwing from blockConcurrencyWhile also resets the DO.
  throw new Error("Gadget restarted because a new connection was added.");
});

Another option would be to store in-memory that the given gatekeeper should be blocked from being accessed -- the mark naturally goes away after restarting. But, this seems more complicated, and blockConcurrencyWhile() should do the trick I think.

[kentonv]

Instead of checking whether any collaborators exist on the sharing graph, we should check whether anyone is actually connected. We can keep a count of the number of live OverseerClientInterface and UseOverseerInterface objects (increment count in constructor, decrement in disposer) in order to decide whether anyone is connected. (We can even count connections by the owner separately from collaborators.)

[kentonv]

I think this is incorrect. The existence of an entry in accountChoices doesn't assert anything about the user's verification status. It simply records the user's choice, so that we don't have to ask them again.

A secondary, though less important, role of accountChoices is to record which gatekeepers may have already had the user registered via a successful addObserver(). Those gatekeepers potentially need later removeObserver() calls to clean up. However, this isn't strictly necessary -- if we accidentally leave a user registered on removeObserver() when they no longer have access, then when they show up in excludeObservers in the future, we can detect that the observer is no longer present and remove them at that time. So existence in accountChoices is more of a hint for this purpose.

But regardless, nothing should be assuming that an entry in accountChoices means the user is actually verified.

Under this understanding, I believe this commit can be removed, since there's not necessarily any need to delete a choice just because it failed re-verification. If we do want to delete it, though, we would also need to call removeObserver() on the gatekeeper to fully remove this observer. But if we do that, we also need to make sure that the observer doesn't have any other live connections open, because if they do, those connections would then be able to bypass excludeObservers.

So I think we should actually just remove this commit.

[kentonv]

Similar to the other comment -- this seems to misunderstand accountChoices.

I think we can leave the record alone. If the gatekeeper ever becomes in-scope again, then the user will probably want to keep the same choice they made earlier.

Note, though, that in the meantime, the user is still registered as an observer on the gatekeeper (and even with the change written here, they still would be -- there's no call to gatekeeper.removeObserver() here). This means they may show up in excludeObservers spuriously. We should have the excludeObservers handling detect that the gatekeeper is no longer in-scope for the user, and then remove the observer from the gatekeeper, even though the observer is still in the workspace's observers table. Currently (unless it changed elsewhere in this PR that I haven't seen yet), when the user shows up in excludeObservers it will actually cause the observation to be blocked, which isn't right if the gatekeeper is no longer in-scope for the user...

[ask-bonk]

[P1] Count admissions before observer verification yields

ensureObserver() snapshots inScope before awaiting the modal/verifier, but this check only sees sessions whose interface constructors have already run. If this is a collaborator’s only open, the owner can add a gatekeeper while configure() is blocked; the count is still zero, so no reset is scheduled, and the old open then returns a build capability that can access the new gatekeeper without any addObserver() check. receiveExternalMessage() never joins this count and has the same interleaving before starting its chat. Hold an admission lease/generation across authorization, or recheck scope immediately before exposing the capability/starting the chat.

[ask-bonk]

[P1] Keep descendant capabilities represented after parent disposal

Capabilities returned by Cap’n Web are independently owned. A use collaborator can retain the UseGadgetClientInterface returned by getGadget(), dispose only this parent, and make the live-session count reach zero; notifyClosed() merely marks that disposal as expected and does not revoke the retained child. The owner can then bind a new restricted resource without scheduling a reset, after which the retained child can call connectToGadget() against the gadget’s current bindings. The build parent at line 9357 has the same issue. Keep the authorization lease alive for descendant capabilities or make every descendant enforce the admitted generation.

[ask-bonk]

[P1] Revalidate scope before removing each observer

outOfScope is classified once, but these removals await one by one. With two excluded observers, the first removeObserver() opens the input gate; while it is pending, the owner can bind this gatekeeper and the second observer can open and successfully re-register. This stale second removal then deletes that fresh registration and the method admits the excluded observation even though the observer can now reach it; later exclusions may omit them too. Recheck the current role/scope before each removal (or serialize cleanup with scope changes).

[ask-bonk]

Submitted 3 actionable inline review findings.

github run

[kentonv]

Bugfix: Don't let a delayed abort swallow a concurrent access change.

This commit is suspicious. It seems to be complaining that a randomly-placed abort() could leave the DO in an inconsistent state, if something else is happening in the meantime that gets interrupted in the middle.

But if that's the case, then that something else is the buggy bit. Every piece of code should be tolerant of a random crash interrupting it. Usually, DO's atomicity guarantees take care of it: any writes performed without an intervening await are automatically coalesced into a transaction. But anything that does writes around an async operation needs to consider what happens if the async operation never returns (due to a crash).

Of course, adding and removing observers from a gatekeeper is inherently async, and we can't even wrap it in a transaction since the gatekeeper itself is modifying its own storage. (I do want to create cross-facet transactions at some point... but we don't have them yet.)

The rule to follow here is: Ensure the gatekeeper overestimates. That is:

• addObserver() must succeed before the observer is granted any access.
• remoteObserver() must not be called until the observer's access has been revoked.

Since it's an over-estimate, in the event of a badly-timed crash, a gatekeeper can end up having an observer registered who doesn't actually have access. That is OK: it only causes that observer to possibly show up in excludeObservers in the future. At that point, the overseer checks and discovers that the excluded observer doesn't have access in the first place, and can then remove them from the gatekeeper, thus bringing it into sync.

In any case, though, I don't quite understand the situation that the first paragraph of the commit description is worried about. If removeCollaborator() has already removed the sharing edge (and the observer has also been removed from the sharing table), then it doesn't really matter if we fail to finish removing the observer from all gatekeepers -- the gatekeepers can be left over-estimating.

[ask-bonk]

• [P1] overseer.ts:2270: Binding or promoting a connection schedules a delayed restart but does not quarantine the widened gadget. During the 100 ms delay, an existing UseGadgetClientInterface can call connectToGadget() and load a fresh facet containing the unverified binding. Block affected gadgets until reset, as addGatekeeper() blocks new IDs.

• [P1] overseer.ts:9426: Disposing the parent removes the build-session count even when independently retained chat/action/console subscriptions remain. Those subscriptions can later receive data from newly added connections without triggering a restart. Each data-bearing subscription needs its own session lease.

• [P1] overseer.ts:4452: #gatekeepersPendingRestart does not cover slash-command access. listSlashCommands() and #prepareChatMessage() access pending gatekeepers directly, allowing a build collaborator to list or invoke a newly added Context skill before observer registration. Filter/assert pending IDs on both paths.

• [P2] overseer.ts:8748: Every non-owner external-message request is counted as build before authorization. A stranger or use collaborator racing a connection addition therefore causes an unnecessary workspace reset. Acquire the long-lived lease only after authorizeCollaborator() returns build; its internal lease already protects verification.

github run