Repository navigation
Move timed allowance lock handling off the main thread - #250
Conversation
There was a problem hiding this comment.
Review round 1: request changes (independent Opus review of 4fb998e, verified by the coordinator)
No P0: no event order lets a timed grant or timed consumption through while locked, asleep or switched out with the setting on. The fence, receipt-order enqueue, receipt-time sleep bound and the setting-off path hold up. One P1 and one P2:
- P1: ordinary contention revokes every timed allowance while unlocked.
lockWithTimedAllowanceScreenSample(+ScreenSession.swift~40-53) validates againststateRevision, which everymutatebumps, including read-only calls (pendingRequests,timedAllowanceDeadline,activeTimedAllowances,withEntry, setting-onwithRuntimeAuthorization). The approval panel's 0.25 s timer, the list/inspector and an agent run's validate/spawn easily invalidate two samples in a row;timedAllowanceScreenSampleFailedthen makesrevokeForScreenBackstopLockedbump the generation and revoke everything.TimedAllowanceScreenSamplingTests.swift~67-88 currently enforces this.- Validate against a dedicated session revision bumped only by
updateTimedAllowanceSession,configureTimedAllowanceScreenState,configureTimedAllowanceSleepOffsetand setting changes inconfigureAgentAccessPolicy; keep the fence-revision check. - A failed sample refuses timed authority for that one operation only: exclude the failed-sample-only case from the backstop (no revocation, no generation bump).
submit(+Requests.swift~94-97) must then also skip auto-approval from an allowance when!timedAllowanceSessionPermitsGrantLocked(). - Replace the test above with one that runs non-session mutations concurrently during sampling and asserts the allowances and generation survive while unlocked.
- Validate against a dedicated session revision bumped only by
- P2: setting-on spawn/validate runs callbacks inside the Vault spawn gate.
TextRuntime.swift~154-166 holds the lease lock and the spawn gate, thenwithRuntimeAuthorization→mutateruns runtime cleanups,timedAllowanceRevokedandoperationStateChanged(SQLite audit writes) before returning. Keep the setting-on path out ofmutate: sample outside the Broker lock, then under the raw lock refuse (throw) if the authorization has timed credentials and the fresh sample or fence does not permit; do not revoke, bump revisions or run callbacks there. Global revocation stays with the queued session update and the next ordinary mutation. Add tests that an Allow/Once spawn with the setting on still succeeds when sampling fails, and that no observer callback or cleanup runs inside the spawn gate.
P3s to take now:
- Clear
timedAllowanceScreenSamplebeforelock.unlock()inmutate(or pass it as a local) so no later reader can see a stale sample. +ScreenSession.swift~20: the comment says only ending events acquire a fence, butbeginalways does; fix the comment or enforce it.+ScreenSession.swift~52:endOnLock ?? endTimedAllowancesOnLockis always true there; assigntrue.
Test gaps to fill:
- a fence begin and finish inside one sample (proves the fence-revision check is needed);
- production fence wiring covered beyond
TimedAllowanceAsyncSessionTests(at least one App test with a realbeginEndingEvent); testDelayedWillSleep…distinguishes receipt-time from execution-time capture in every iteration, not onlyrealSleep == true.
|
Addressed review round 1 in
Validation: |
There was a problem hiding this comment.
Review round 2: small changes (independent Opus delta review of f27a8cf, verified)
Round 1 is fixed: no P0–P2. Every timed path refuses on a failed sample, a contention-only failure never revokes or bumps the generation, and the setting-off path is unchanged. Last round, small and in scope:
- Do not sample for spawns without timed credentials.
+RuntimeAuthorization.swift~5: with the setting on, once/allow spawns still make up to two WindowServer calls while the caller holds the Vault gate and lease lock (shared withisPaused()). Peek under the Broker lock first and sample only whenruntimeAuthorizations[id]?.timedCredentialIDsis non-empty. Test that a once-only spawn with the setting on makes zero provider reads. - Bump the session revision only when
endTimedAllowancesOnLockchanges (+Authentication.swift~12-17); the other settings do not affect sample validity. - Comment at
permits/clearTimedAllowanceSleepIfProvenLocked(+ScreenSession.swift~83): it may clear sleeping after proven sleep even on the runtime gate path; say so.
Tests to add or tighten:
- consumption under a failed sample (
consumeForRuntime,reserveConsumption,consumeApproved(now:)) refuses timed-approved entries and leaves once-only entries alone; - a failed sample while the fence is pending revokes (the
|| fence.pendingbranch) in both the backstop and policy apply; - setting off:
withRuntimeAuthorizationwith a configured provider makes zero provider reads; testEveryLiveSampleIncludingPolicyEnableAndReplacement…: assertprobe.readsafterconfigureAgentAccessPolicy;testSettingOnAllowAndOnceSpawnSurviveFailedSample…: drop or fix the allow case that never reaches the Broker;testRepeatedSessionRaces…: pin thedeciderejection to.invalidDecision.
Known and accepted, not for this PR: a contention failure during submit turns an auto-approval into a pending entry (rare now that only session events and the lock setting change the revision); beginSpawnAuthorization → configureAgentAccessPolicy still runs a full mutate inside the gate when a remote policy change is detected (predates this PR).
|
Addressed the final review round in
Validation: |
Closes #247
Summary
Diff stat
25 files changed, 1003 insertions(+), 116 deletions(-), relative to
origin/mainat774fd37c1916a13d693a927f5113d00e8d576d45.Tests
swift build.swift test --filter '(ApprovalStateMachine|TimedAllowance|ReadProjectionAllowance)': 131 passed, 0 failed, 0 skipped (Vault 23, Broker 62, App 46).afb85bc3eb99003df6ee512bc9a714b17336913e): 1,598 Swift tests, 3 skipped, 0 failed; 207 Automation tests. A separate local baseline run is NOT_RUN.92e948c92b46ef15ab687ae431eb53c488661239(tree0fb2da66010479bc6f16742adfcb27e60bbf3e41):build-and-test,basic-ui-flowsandrelease-sdk-window-checkall succeeded, with no failed steps.Checks
python3 scripts/check_hygiene.py(all seven rule counts zero; source files under 600 lines).python3 scripts/check_module_deps.py.git diff --check.origin/mainbefore push; no additional main commits were present..build(about 1.08 GiB), validation logs and downloaded CI logs removed after reading; about 231 GiB free, with no active task build/test process.Deviations and questions