Reset persistent and in-memory scheduling state safely

[HituziANDO]

Summary

• reset persistent and in-memory version-check, review-request, and release-note scheduling state synchronously for the active environment
• invalidate in-flight and queued operations with environment-scoped execution tokens so stale work cannot restore state after a reset
• make dictionary mutations consistently synchronized while preserving environment isolation and existing override behavior
• add regression coverage for built-in conditions, reset lifecycle interleavings, runtime snapshots, and concurrent dictionary mutation

Testing

• iOS workspace test suite: 41 tests passed
• Thread Sanitizer targeted suite: 9 tests passed with no reported data races
• macOS universal framework build for arm64 and x86_64
• SwiftFormat verification
• git diff --check

Manual validation

• Real App Store network requests, StoreKit review UI, and platform alert presentation were not exercised manually.

Closes #15

[HituziANDO]

Review

Verified locally

• iOS test suite (iPhone 17, iOS 26.5): 41 passed, 0 failures
• SwiftyUpdateKit macOS scheme, platform=macOS: BUILD SUCCEEDED
• SwiftFormat 0.53.7 (.codeformat/swiftformat --lint): 0/18 files require formatting

Overall: the core mechanism is sound. Per-environment generation counters plus environment-scoped execution keys are the right shape for making reset() invalidate in-flight and queued work, and I traced every .started path — each one is matched by exactly one finish(), including the post-reset case where a stale finishExecution correctly declines to evict a replacement execution. Nothing below is a blocker. My main concerns are the amount of new machinery relative to the problem, and a few behavior changes that aren't documented.

---

1. Six near-identical conformance extensions (Medium)

RequestReviewCondition.swift:165-245 and VersionCheckCondition.swift:112-159 repeat the same 4–5 forwarding methods six times — roughly 110 lines that all read schedule.<same call>. An internal marker protocol collapses this:

protocol DailyScheduleBacked: AnyObject {
    var dailySchedule: DailySchedule { get }
}

extension ReviewRequestExecutionControlling where Self: DailyScheduleBacked {
    func reviewRequestPreflightToken(in userDefaults: SUKUserDefaults) -> SchedulingExecutionToken {
        dailySchedule.executionToken(in: userDefaults)
    }
    // ...
}

Each condition class then needs one line (var dailySchedule: DailySchedule { schedule }) plus the conformance. Same for VersionCheckExecutionControlling.

2. SUKSchedulingStateStore now has two parallel APIs, and half is unused in production (Medium)

DailySchedule.swift:29-49. Production code only ever calls the for context: variants — DailySchedule uses them exclusively, and SUK.reset() uses removeValue(for:). The key-based trio (set(_:forKey:), integer(forKey:), removeValue(forKey:)) survives only for TestSchedulingStateStore and the ResetTests assertions.

That matters because those implementations resolve SUKUserDefaults.standard ambiently (DailySchedule.swift:52-62, 100-102) — exactly the pattern this PR removes everywhere else. A future caller who picks the key-based overload silently reintroduces the ambient-environment bug, with no compiler signal. I'd collapse the protocol to the context-based API only and port the test double and assertions.

3. Arbitrary work runs under a process-global, non-recursive lock (Medium)

DailySchedule.swift:210-218 (performStateAccessIfCurrent) and 231-241 (reset) both invoke action() while holding SchedulingExecutionGate.lock. Those actions do UserDefaults reads/writes, JSON encode/decode (ReleaseNotes.update), and a dispatch_barrier_sync on sharedDictionary. Two consequences:

• checkNewRelease runs on the main queue when reached via SUK.swift:558, so its performStateAccessIfCurrent calls (SUK.swift:692, 699) can block the main thread on a lock held by a background thread doing UserDefaults I/O.
• NSLock is not recursive. No framework code re-enters the gate from inside action() today, but the path is reachable from outside the framework — an app observing UserDefaults.didChangeNotification (posted synchronously on the mutating thread) and calling SUK.reset() from the observer deadlocks.

Narrowing the critical section would weaken the atomicity guarantee this PR is built on, so I'm not asking for that. But please at least state the invariant in a comment ("action must not re-enter the gate"), and consider whether reset()'s userDefaults.removeObject(forKey: SwiftyUpdateKitLatestAppVersionKey) (SUK.swift:353) really needs to be inside the lock.

4. The thread-local token is undocumented and fails silently across thread hops (Medium)

SchedulingExecutionScope (DailySchedule.swift:116-134) passes the token as invisible thread state, consumed by DailySchedule.recordCurrentDate() (DailySchedule.swift:286-292) and currentContext (336-339).

recordSuccessfulVersionCheck(), recordReviewRequestAttempt(), and shouldRequestReview() are all open. If an app subclass hops threads inside an override — DispatchQueue.global().sync { }, an actor, an await whose continuation resumes elsewhere — the token is gone and recordCurrentDate() falls back to SUKUserDefaults.standard with no generation check. That is precisely the stale write this PR exists to prevent, and it fails open rather than closed.

I understand TLS was chosen so the parameterless open methods stay source-compatible; that's a reasonable trade. Please say so in a comment and state the constraint. More broadly: the ~400 new lines of concurrency code in DailySchedule.swift currently carry zero explanatory comments, and this is the part of the codebase where a future reader most needs the "why".

5. Behavior changes worth documenting or reconsidering

a. An already-visible update alert's button becomes a no-op after reset() — SUK.swift:188-196. Guarding the presentation with isCurrent() is right. Guarding the tap handler means a user taps "Update" on an alert that is on screen and nothing at all happens. I'd check isCurrent() only before presenter(...) and let a presented alert honor its own action.

b. .inProgress now takes precedence over ineligibility. Previously beginVersionCheck() evaluated shouldCheckVersion() first, so an ineligible condition returned .notEligible → next(nil) → noop?(). Now beginExecution is consulted first (SUK.swift:437-446) and eligibility is evaluated after .started, so a second checkVersion during an in-flight lookup returns .inProgress and noop never fires. The window is narrow — finish() runs immediately after recording — but callers treating noop as "nothing happened" lose the signal there.

c. .notEligible no longer means "the condition said no." SchedulingExecutionGate.beginExecution returns it only for a stale or environment-mismatched token (DailySchedule.swift:186-189); real ineligibility is handled in the .started branch. Both branches then log the identical "Skips the version check." (SUK.swift:474 and 486), so logs can't distinguish "condition declined" from "reset invalidated this operation". Renaming to .stale/.invalidated and logging distinctly would make field logs far more useful.

d. SUK.initialize does not bump the generation. A review request or update alert queued under .production still completes against production state after the app re-initializes into .development. testQueuedUpdateAlertUsesCapturedConfigurationAndStoreURL asserts this, so it reads as deliberate — worth one sentence in the initialize(withConfig:log:) doc comment, since reset() is now the only thing that invalidates.

e. showUpdateAlert() and the enqueued alert disagree about which config wins. The public method reads the live runtime config (SUK.swift:163-177); the internal path uses the config captured at lookup time. Both are defensible, but they can now open different store URLs from the same app.

f. README. The environment scoping is documented, but not the headline user-visible change: reset() now cancels in-flight and queued operations, including a pending update alert. That deserves a line.

6. Minor

• SUK.swift:524 — didRecord is misleading. recordSuccessfulVersionCheck(_:) returns isCurrent(), and returns true even when the condition doesn't conform to VersionCheckSuccessRecording and nothing was recorded. isStillCurrent reads truer.
• SUK.swift:528-535 — three consecutive guard context.isCurrent() separated only by a writeLog call. The one at 535 guards no state change.
• SUK.swift:566 — requestReviewIfNeeded is now reached only from tests; all three public entry points go through enqueueReviewRequest. Either drop it or make it explicit that it's a test seam.
• AtomicDictionary.setValue: async(.barrier) → sync(.barrier) is fine and makes writes visible on return, but it introduces a deadlock if any future code calls setValue/removeValue from inside a value(forKey:) block. One line of rationale would help.
• Over the 100-column .swiftformat limit (SwiftFormat can't wrap these, so lint stays green): SUK.swift:455, 489, 668; tests 428, 452, 527.

7. Tests

• RoundTwoRegressionTests (tests:279) is named after a review round, not after behavior. Something like SchedulingInvalidationTests survives the loss of that context.
• testDailyVersionCheckCallsOpenOverrides / testLaunchingAndDailyVersionCheckCallsOpenOverrides / testReviewRequestCallsOpenOverrides — "CallsOpenOverrides" is opaque on a failure report. ...InvokesSubclassOverrides says it.
• testResetRestoresAllReviewRequestConditions (tests:100) asserts roughly twenty things across four condition types with SUK.reset() interleaved throughout. A failure is hard to localize. Splitting per condition type would cost little.
• ResetTests.setUpWithError sets .test, but most tests then call initializeSUKForSchedulingTests(), which switches to .production and writes the unsuffixed production keys into the host's real UserDefaults. Cleanup only happens in tearDown, so a hard failure leaves production-scoped defaults behind on the simulator. Passing development: true would avoid it everywhere except testResetDoesNotRemoveStateFromAnotherEnvironment, where cross-environment coverage is the whole point.

[HituziANDO]

Re-review (699f626)

Verified locally

• iOS test suite (iPhone 17, iOS 26.5): 47 passed, 0 failures (was 41)
• SwiftyUpdateKit macOS scheme, platform=macOS: BUILD SUCCEEDED
• SwiftFormat 0.53.7 (.codeformat/swiftformat --lint): 0/18 files require formatting
• Lines over the 100-column limit: every newly introduced one is wrapped; the remainder are pre-existing doc-comment URLs in SUK.swift:280/299/314 and tests 45/55

All seven items from the previous round are addressed, and the two behavioral ones came with regression tests. LGTM.

---

Previous findings

#	Finding	Resolution
1	Six near-identical conformance extensions	DailyScheduleBacked plus constrained protocol extensions on both VersionCheckExecutionControlling and ReviewRequestExecutionControlling. ~110 lines down to ~20
2	SUKSchedulingStateStore dual API, half unused in production	Collapsed to the three context-based methods. The ambient SUKUserDefaults.standard lookups are gone from both stores, and TestSchedulingStateStore now keys on context.storageKey
3	Arbitrary work under a global non-recursive lock	Documented rather than restructured — the re-entrancy invariant is stated on SchedulingExecutionGate, and the comment on SwiftyUpdateKitLatestAppVersionKey explains why that removal has to stay inside the critical section. That is the right call; narrowing the section would have weakened the atomicity the design depends on
4	Thread-local token undocumented	SchedulingExecutionScope now states the source-compatibility rationale and the thread-hop constraint explicitly
5a	Presented alert's Update button became a no-op	isCurrent() removed from the tap handler; covered by testPresentedUpdateAlertActionStillOpensCapturedStoreURLAfterReset
5b	.inProgress outranked ineligibility, dropping noop	Eligibility moved ahead of beginExecution, restoring the original precedence; covered by testIneligibleVersionCheckCallsNoopWhileAnotherLookupIsInProgress
5c	.notEligible no longer meant "condition declined"	Renamed to .invalidated, with versionCheckInvalidatedLog separated from the declined-condition message
5d/5e/5f	Doc gaps and showUpdateAlert inconsistency	initialize and reset doc comments plus two README sections; showUpdateAlert now snapshots at call time and opens the captured storeURL, matching the enqueued path
6	didRecord, redundant guards, test-only method	isStillCurrent; two redundant isCurrent() guards dropped; requestReviewIfNeededForTesting renamed and documented; rationale comment on AtomicDictionary.setValue
7	Test naming, oversized test, production-key writes	SchedulingInvalidationTests; ...InvokesSubclassOverrides; testResetRestoresAllReviewRequestConditions split into four; initializeSUKForSchedulingTests now defaults to development: true, so only the two tests that deliberately exercise environment isolation touch production-scoped keys. SchedulingConditionTests also gained setUp/tearDown

I re-checked order-independence across the suite: every test class sets its environment in setUpWithError, so the added setUp/tearDown in SchedulingConditionTests does not leak into SwiftyUpdateKitTests.

---

New observations from this round

All minor, none blocking.

checkVersion lost a cheap staleness check. The guard context.isCurrent() that used to sit between the eligibility branch and the lookup is gone (SUK.swift:483-494). If reset() lands between beginExecution returning .started and the request going out, one App Store request is issued and then fully discarded — recordSuccessfulVersionCheck returns false, finish() runs, and every downstream branch bails on isCurrent(). So it is safe, just a wasted network round trip in a microsecond-wide window. One line before performVersionLookup would close it.

The .invalidated branch in checkVersion is close to dead code (SUK.swift:498-507). beginExecution returns .invalidated(preflightToken), whose executionKey is nil, so the context.finish() there can never remove anything. The branch is also barely reachable: preflightContext.isCurrent() a few lines above checks the same generation, and the environment cannot differ because both sides derive from the same runtimeContext.userDefaults. Fine to keep as defense in depth — just worth not reading that finish() as doing work.

shouldRequestReview() now runs before the execution slot is acquired (SUK.swift:621-628). For the two SkipFirstDay conditions that call also writes the first-day date, so the date can now be recorded on a path where beginReviewRequest subsequently returns .inProgress. Reset safety is unaffected — the write still goes through the generation-checked performStateAccessIfCurrent — and recordCurrentDate is idempotent, so there is no functional impact. Noting it because the symmetric version-check path has no writing condition, which makes the asymmetry easy to miss later.

requestReviewIfNeededForTesting ships in the release framework. The rename makes the intent unambiguous, which was the point. If you want it out of the product binary entirely, #if DEBUG around it still works for @testable import.