Conversation
|
||||||||||||||||||
|
||||||||||||||
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #4180 +/- ##
==========================================
+ Coverage 67.81% 67.94% +0.12%
==========================================
Files 254 254
Lines 23040 23083 +43
==========================================
+ Hits 15625 15684 +59
+ Misses 7415 7399 -16
🚀 New features to boost your workflow:
|
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Reviewed at commit 56b708f. This reworks the DPoP nonce cache from a flat
[compositeKey: String] dictionary into per-scope state ([scope: ScopeCache]), replacing the
old linear scan for "latest nonce in this scope" with an explicit latestHTU pointer, and adds
a 32-entry-per-scope bound with least-recently-written eviction.
Correctness of the three headline claims, verified against the code (not just the PR
description):
- Exact lookup, latest-for-scope, and clear-for-scope are all genuinely O(1) expected time now
— direct dictionary lookups, no scan.clear(forScope:)in particular improves beyond what
was asked: it now only touches the target scope's own entry, versus the old code's O(n) walk
over every key in the whole cache across all scopes. - The old "latest" lookup was relying on Swift dictionary iteration order, which is undefined —
that was the real root cause behind the reported bug, not just the scan cost. The new
latestHTUfield, updated on every write, replaces that with an explicit, correct pointer. - The eviction design correctly distinguishes "oldest write" from "oldest currently-live entry":
evictIfNeededonly evicts a FIFO record ifentries[htu]?.sequencestill matches that
record's sequence, so a HTU that's been rewritten since its FIFO record was queued survives —
this is what makestest_givenRewrittenNonce_whenCapacityExceeded_thenRewriteRefreshesEvictionOrder
pass and is the subtle part of the LRU-ish design that would be easy to get wrong.
Trace against a nonce rotation exceeding 32 entries while a retry is in flight: every real
caller (DPoPRequestDecorator.decorate, invoked from the OAuth coordinator, the token-endpoint
helper, and SFRestAPI's resource-call retry path) reads the cache fresh immediately before
building each proof rather than holding a nonce value across a round-trip, and the entry just
written can never be the one evicted in the same call (eviction only pops from the FIFO's
oldest end, and the just-written entry always has the newest sequence number). Worst case for a
resource-server nonce evicted between attempts is a harmless fallback to the token-endpoint
nonce or one extra use_dpop_nonce round-trip — not a wrong or missing proof.
Trace against the "eviction bookkeeping itself stays bounded" claim: confirmed by
arithmetic for the pathological case (one HTU rewritten indefinitely, which never trips the
main 32-entry eviction): compactWriteOrderIfNeeded's liveRecordCount > 64 guard caps how far
the writeOrder FIFO can grow between compactions, and compaction rebuilds it down to
entries.count. The self-referential concern in the PR description (an eviction mechanism that
itself leaks) doesn't materialize.
Test coverage: the four new tests each map to one of the properties above (deterministic
latest-write, capacity eviction, rewrite-refreshes-eviction-order, concurrent bounded capacity)
and the assertions in each match what the test name claims — no mismatches or vacuous
assertions found. The concurrent test's per-iteration _ = latest(forScope:) call after each
write is worth noting as deliberate synchronization (GCD guarantees a .sync enqueued after an
.async on the same queue drains everything ahead of it) rather than incidental — that's what
makes the post-loop assertion deterministic instead of racy.
Public API: unchanged — every @objc selector on DPoPNonceCache keeps its name and
signature. The renamed helper (cacheKey -> scopeKey) is private static.
Cross-platform: no Android twin for this exact nonce-cache finding was found in this
review batch, so there's no parity check to perform here.
Approving on the merits above.
This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
This branch has not been deployed
3 participants
Description
Reworks the process-lifetime iOS DPoP nonce cache from a flat composite-key
dictionary into scope-indexed state.
expected time.
eviction.
thread-safety model, and account lifecycle cleanup.
Spec PR: https://git.soma.salesforce.com/SalesforceMobileSDK/SalesforceMobileSDK-Workspace/pull/112
Resolves
W-24269744
Testing
Tests in the PR
Manual Tests
No manual authentication flow is required for this internal cache data-structure
change.
SFSDKDPoPTestson iPhone 16 / iOS 18.6.concurrent bounded-capacity coverage.
git diff --checkand reviewed the diff for public API changes andsensitive logging.