test(approvals): batch persistent seed grants into one Ask per case - #1989
Merged
Conversation
ShellPolicyEvidenceFixtureTests.Policy_fixtures_execute_through_the_coordinator flakes on Windows CI with an AskTimeoutException (5s) during harness seeding. Each persistent grant was seeded with a separate Ask, and each Ask's handler does synchronous cache-bypassing disk I/O (WriteThrough + Flush(flushToDisk: true) + File.Move) on the shared default dispatcher. Under full-suite parallel load (Defender scanning fresh %TEMP% store files), the heaviest case (D10, 5 seeds) occasionally pushed one Ask past the hard 5s wall clock. The actor's RecordStructuredToolApproval handler already persists a whole grant list in a single locked atomic write (ToolApprovalStore.AddApprovals -> one SaveLocked), so grouping seeds by audience into one Ask per case is byte-for-byte equivalent while cutting seed-phase file rewrites from 24 to 10. Test-only change; no production behavior touched. Verified: all 330 tests in ShellPolicyEvidenceFixtureTests + ShellApprovalDispositionMatrixTests pass in ~2s. Refs run 32149410006 / job 95751209585 (commit 00d93eb).
Adversarial review nit: ToolApprovalStore.AddApprovals stamps CreatedAt per entry, so under a real clock the batched flow stamps all grants at one instant vs distinct instants. Unobservable in this suite (fixture tests use a frozen FakeTimeProvider; no harness test asserts seed timestamps), but the comment should not overclaim byte-for-byte equality. Also note: the partial-batch-failure case (one malformed grant failing an audience group) is real but not a regression -- RecordStructuredToolApproval validates all grants and returns InvalidData, which AkkaToolApprovalService throws on in both old and new flows, so no caller can observe a silent partial-seed difference.
This was referenced Aug 18, 2026
Aaronontheweb
marked this pull request as ready for review
August 20, 2026 14:45
Aaronontheweb
enabled auto-merge (squash)
August 20, 2026 14:45
Aaronontheweb
commented
Aug 20, 2026
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM - perf optimization
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.
What
Fixes a flaky
AskTimeoutExceptioninShellPolicyEvidenceFixtureTests.Policy_fixtures_execute_through_the_coordinatoron Windows CI.Refs: run 32149410006 / job 95751209585 (commit
00d93eb1).Root cause
Each persistent grant was seeded with a separate
Ask, and eachAsk's handler (ToolApprovalActor→ToolApprovalStore.AddApprovals) does synchronous, cache-bypassing disk I/O on the shared default dispatcher:WriteThrough+Flush(flushToDisk: true)+File.Moveunder an exclusive lock.Under full-suite parallel load on
Test-windows-latest(Defender real-time-scanning each freshtool-approvals.jsonin a new%TEMP%tree), the heaviest fixture case — D10, with 5 persistent seeds — occasionally pushed one seedAskpast the hard 5-second wall clock. The log confirms D02–D09 completed correctly before the timeout hit D10's seeding phase.Ruled out (with code evidence):
lockTimeout: TimeSpan.Zero, soAcquireLockis single-attempt fail-fast; contention throws, never blocks 5s.Askis awaited beforeGracefulStop; noAskis in flight during actor recreation.Fix
Group persistent seeds by audience into one
Askper case instead of one per grant. The actor'sRecordStructuredToolApprovalhandler already persists a whole grant list in a single locked atomic write (ToolApprovalStore.AddApprovals→ oneSaveLocked), so the resulting store state is equivalent to N sequential seed messages — it still exercises the real production path.Cuts seed-phase file rewrites from 24 → 10 (D10: 5 → 1), removing the per-case worst-case latency from the 5s budget.
Test-only change. No production behavior touched; the hardcoded 5s
Asktimeout inAkkaToolApprovalServiceis intentionally left alone.Equivalence note (from adversarial review)
The only state divergence vs N sequential asks is per-entry
CreatedAtstamping under a real clock (a batch stamps all grants at one instant). This is unobservable here: fixture tests run on a frozenFakeTimeProvider, and no harness test asserts seed timestamps. The partial-batch-failure case (one malformed grant failing an audience group) is real but not a regression —RecordStructuredToolApprovalvalidates all grants and returnsInvalidData, whichAkkaToolApprovalServicethrows on in both old and new flows, so no caller can observe a silent partial-seed difference.Verification
ShellPolicyEvidenceFixtureTests+ShellApprovalDispositionMatrixTestspass in ~2s (the flaky path was ~23s+ before timing out).The flake only reproduces under parallel Windows load, so CI on Windows is the real proof. Suggested stress loop if it recurs:
Note (not addressed here)
ToolApprovalActordoing synchronous disk I/O inside message handlers is a latent production risk this test exposed. A follow-up could assign it a dedicated dispatcher or move the store calls off the actor thread — out of scope for this test-only fix.