Commit `cc6358f98a`

cc6358f98a260e4e3ffed4a51d82c2d71095397f

parent: 2b5b408a5c

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-06 22:39 UTC

Close the compare-refs stale-result race with a generation token

compare() now captures a generation counter before awaiting and discards
its result if a base/head change has bumped it in the meantime, so a
superseded request can no longer overwrite the current ref pairing's
state, mergeBase, or truncated flag.

Layout: unified · split

gitbay/Repos/CompareViewModel.swift +12

		@@ -36,6 +36,14 @@ final class CompareViewModel {
36	36	private(set) var refs: RepoRefs?
37	37	private(set) var refsError: String?
38	38
	39	/// Bumped by `resetResults()` (so on every `base`/`head` change) and
	40	/// captured by `compare()` before it awaits. A response that arrives
	41	/// after the generation has moved on belongs to a ref pairing nobody
	42	/// is looking at any more, so it's discarded rather than written —
	43	/// otherwise a slow request for an old ref pair could land after a
	44	/// fast one for the current pair and overwrite it.
	45	private var generation = 0
	46
39	47	private let client: GitbayClient
40	48	let repoPath: String
41	49
		@@ -60,6 +68,7 @@ final class CompareViewModel {
60	68	/// (or a ref changed mid-flight) can never pair a stale merge base or
61	69	/// truncation notice with the new state.
62	70	private func resetResults() {
	71	generation += 1
63	72	state = .empty(Self.notComparedYet)
64	73	mergeBase = nil
65	74	truncated = false
		@@ -70,11 +79,13 @@ final class CompareViewModel {
70	79	let head = head.trimmingCharacters(in: .whitespaces)
71	80	guard !base.isEmpty, !head.isEmpty else { return }
72	81
	82	let requestGeneration = generation
73	83	state = .loading
74	84	mergeBase = nil
75	85	truncated = false
76	86	do {
77	87	let diff = try await client.read(["repo", "diff", repoPath, base, head], as: RepoDiff.self)
	88	guard requestGeneration == generation else { return }
78	89	truncated = diff.truncated
79	90	mergeBase = diff.mergeBase
80	91	let parsed = UnifiedDiff.parse(diff.patch)
		@@ -82,6 +93,7 @@ final class CompareViewModel {
82	93	? .empty("No differences between \(base) and \(head).")
83	94	: .loaded(parsed)
84	95	} catch {
	96	guard requestGeneration == generation else { return }
85	97	state = .from(error)
86	98	}
87	99	}

gitbayTests/CompareTests.swift +153 −1

		@@ -17,12 +17,90 @@ private func argvFrom(_ url: URL) -> [String] {
17	17	.queryItems?.filter { $0.name == "argv" }.compactMap(\.value) ?? []
18	18	}
19	19
20		private let patchJSON = """
	20	nonisolated private let patchJSON = """
21	21	{"protocol_version":1,"data":{"base":"aaa111","head":"bbb222","merge_base":"ccc333",\
22	22	"patch":"diff --git a/x.txt b/x.txt\\n--- a/x.txt\\n+++ b/x.txt\\n@@ -1 +1 @@\\n-old\\n+new\\n",\
23	23	"truncated":false},"exit_code":0}
24	24	"""
25	25
	26	/// `StubProtocol` answers a request as soon as it arrives, and the actual
	27	/// delivery back onto the main actor still crosses a real thread — so
	28	/// polling its request log cannot promise "the response has not landed
	29	/// yet" at the instant a test checks it. `GatedProtocol` pins that instant
	30	/// down: `startLoading` blocks the request until the test explicitly
	31	/// releases it, so a ref mutation made between arrival and release is
	32	/// provably made while the request is still outstanding. Scoped per test
	33	/// the same way `StubProtocol` is, via a marker header, so parallel tests
	34	/// don't share a gate.
	35	nonisolated private final class GatedProtocol: URLProtocol, @unchecked Sendable {
	36
	37	final class Box: Sendable {
	38	fileprivate let id = UUID().uuidString
	39	private let gate = Mutex<(continuation: CheckedContinuation<Void, Never>?, release: DispatchSemaphore?)>((nil, nil))
	40
	41	func session() -> URLSession {
	42	let configuration = URLSessionConfiguration.ephemeral
	43	configuration.protocolClasses = [GatedProtocol.self]
	44	configuration.httpAdditionalHeaders = [GatedProtocol.marker: id]
	45	return URLSession(configuration: configuration)
	46	}
	47
	48	/// Suspends until the in-flight request reaches `startLoading`,
	49	/// then returns the semaphore that must be signaled to let its
	50	/// (canned, successful) response through.
	51	func awaitArrival() async -> DispatchSemaphore {
	52	let release = DispatchSemaphore(value: 0)
	53	await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in
	54	gate.withLock { $0 = (continuation, release) }
	55	}
	56	return release
	57	}
	58
	59	fileprivate func arrived() -> (CheckedContinuation<Void, Never>?, DispatchSemaphore?) {
	60	gate.withLock { state in
	61	let result = state
	62	state.continuation = nil
	63	return result
	64	}
	65	}
	66	}
	67
	68	private static let marker = "X-Gated-Box"
	69	private static let boxes = Mutex<[String: Box]>([:])
	70
	71	static func box() -> Box {
	72	let box = Box()
	73	boxes.withLock { $0[box.id] = box }
	74	return box
	75	}
	76
	77	override static func canInit(with _: URLRequest) -> Bool { true }
	78	override static func canonicalRequest(for request: URLRequest) -> URLRequest { request }
	79
	80	override func startLoading() {
	81	guard let id = request.value(forHTTPHeaderField: Self.marker),
	82	let box = Self.boxes.withLock({ $0[id] }) else {
	83	client?.urlProtocol(self, didFailWithError: URLError(.resourceUnavailable))
	84	return
	85	}
	86	let (continuation, release) = box.arrived()
	87	continuation?.resume()
	88	release?.wait() // blocks this background thread until the test releases it
	89
	90	let response = HTTPURLResponse(
	91	url: request.url!,
	92	statusCode: 200,
	93	httpVersion: "HTTP/1.1",
	94	headerFields: ["Content-Type": "application/json"]
	95	)!
	96	client?.urlProtocol(self, didReceive: response, cacheStoragePolicy: .notAllowed)
	97	client?.urlProtocol(self, didLoad: Data(patchJSON.utf8))
	98	client?.urlProtocolDidFinishLoading(self)
	99	}
	100
	101	override func stopLoading() {}
	102	}
	103
26	104	struct RepoDiffDecodingTests {
27	105
28	106	@Test func decodesEveryField() throws {
		@@ -175,6 +253,80 @@ struct CompareViewModelTests {
175	253	}
176	254	}
177	255
	256	/// The race from the retarget bug: a compare is outstanding, the user
	257	/// changes a ref before it resolves, and the outstanding request's
	258	/// result must not land — it belongs to a ref pairing nobody is
	259	/// looking at any more. `GatedProtocol` makes the interleaving exact
	260	/// rather than probable: the request is provably still unanswered when
	261	/// the ref changes, because nothing has let its response through yet.
	262	@Test func aStaleCompareDoesNotOverwriteANewerRef() async throws {
	263	let box = GatedProtocol.box()
	264	let client = GitbayClient(
	265	instance: try GitbayInstance(url: "https://gitbay.org"),
	266	token: "test-token",
	267	session: box.session()
	268	)
	269	let model = CompareViewModel(client: client, repoPath: "krz/gitbay")
	270	model.base = "aaa111"
	271	model.head = "v1"
	272
	273	let task = Task { await model.compare() }
	274	let release = await box.awaitArrival() // the v1 request has arrived and is now blocked
	275
	276	guard case .loading = model.state else {
	277	Testing.Issue.record("expected .loading while the request is outstanding, got \(model.state)")
	278	release.signal()
	279	await task.value
	280	return
	281	}
	282
	283	model.head = "v2" // ref changed while the v1 request is still in flight
	284	release.signal() // let the v1 response through now that it's stale
	285	await task.value
	286
	287	#expect(model.state.value == nil)
	288	#expect(model.mergeBase == nil)
	289	#expect(model.truncated == false)
	290	}
	291
	292	/// A superseded compare must not leave the screen presenting as
	293	/// in-flight forever: the ref change has to clear `.loading`
	294	/// immediately, and the discarded response arriving afterward must
	295	/// not put it back.
	296	@Test func aSupersededCompareStopsPresentingAsInFlight() async throws {
	297	let box = GatedProtocol.box()
	298	let client = GitbayClient(
	299	instance: try GitbayInstance(url: "https://gitbay.org"),
	300	token: "test-token",
	301	session: box.session()
	302	)
	303	let model = CompareViewModel(client: client, repoPath: "krz/gitbay")
	304	model.base = "aaa111"
	305	model.head = "v1"
	306
	307	let task = Task { await model.compare() }
	308	let release = await box.awaitArrival()
	309
	310	guard case .loading = model.state else {
	311	Testing.Issue.record("expected .loading while the request is outstanding, got \(model.state)")
	312	release.signal()
	313	await task.value
	314	return
	315	}
	316
	317	model.head = "v2" // ref changed while the v1 request is still in flight
	318
	319	if case .loading = model.state {
	320	Testing.Issue.record("ref change did not clear .loading immediately")
	321	}
	322	release.signal()
	323	await task.value
	324
	325	if case .loading = model.state {
	326	Testing.Issue.record("superseded compare's response left the screen stuck presenting as in-flight")
	327	}
	328	}
	329
178	330	/// A second compare that fails must not show the first compare's
179	331	/// merge base or truncation notice beside the failed state.
180	332	@Test func aFailedRecompareDropsTheStaleResult() async throws {

Commit cc6358f98a

Commit `cc6358f98a`