fix(ios): give scheduleNotification's add a deadline, starting on the grant

packages/zig/src/bridge_mobile_notifications.zigmodified+190-67

Changes to packages/zig/src/bridge_mobile_notifications.zig

		⋯273 linesShow the 273 lines above this hunk@@ -274,12 +274,23 @@ pub const NotificationsBridge = struct {
274	274	errdefer schedule.deinit(self.allocator);
275	275
276	276	const ticket = ios_async.acquire(A.schedule_notification) orelse return poolFull();
	277	+ errdefer ios_async.abandon(ticket);
277	278
278		- publishScheduleCall(ticket, sels, schedule);
	279	+ const block = schedule_calls.claim(ticket, .{ .sels = sels, .schedule = schedule }) orelse {
	280	+ std.log.warn(
	281	+ "scheduleNotification: all {d} completion blocks are still owed a completion " ++
	282	+ "UserNotifications never delivered; refusing rather than reusing one",
	283	+ .{ios_pending.block_count},
	284	+ );
	285	+ return poolFull();
	286	+ };
279	287
	288	+ // No deadline yet. Stage one is the authorization prompt, and there is
	289	+ // a person reading it; `authorizationFired` starts the clock on the
	290	+ // grant, over the write that nobody is waiting in front of.
280	291	const Fn = *const fn (Id, Id, c_ulong, *anyopaque) callconv(.c) void;
281	292	const func: Fn = @ptrCast(&objc.objc_msgSend);
282		- func(center, sel_request, un_options_alert_sound_badge, boolErrorBlock(ticket));
	293	+ func(center, sel_request, un_options_alert_sound_badge, boolErrorBlock(block));
283	294	}
284	295	};
285	296
		⋯556 linesShow the 556 lines above this hunk@@ -842,32 +853,25 @@ const ScheduleSels = struct {
842	853	};
843	854
844	855	const ScheduleCall = struct {
845		- ticket: ios_async.Ticket,
846	856	sels: ScheduleSels,
847	857	schedule: Schedule,
848	858	};
849	859
850		-var schedule_calls: [ios_async.max_in_flight]?ScheduleCall = @splat(null);
851		-/// Still the old shape: `scheduleNotification` is two stages, and its deadline
852		-/// has to start after the authorization prompt rather than at dispatch, which
853		-/// is its own change (#223).
854		-var pending_mutex: compat_mutex.Mutex = .{};
855		-
856		-fn publishScheduleCall(ticket: ios_async.Ticket, sels: ScheduleSels, schedule: Schedule) void {
857		- pending_mutex.lock();
858		- defer pending_mutex.unlock();
859		- schedule_calls[ticket.index] = .{ .ticket = ticket, .sels = sels, .schedule = schedule };
860		-}
	860	+/// Both stages park here, for the reason `getPendingNotifications` does:
	861	+/// keyed by reply-slot index, a deadline would free slot k, the next schedule
	862	+/// would lease slot k and get the same block, and the first one's late
	863	+/// completion would answer it (#223).
	864	+var schedule_calls: ios_pending.Table(ScheduleCall) = .{};
861	865
862		-fn takeScheduleCall(index: u5) ?ScheduleCall {
863		- pending_mutex.lock();
864		- defer pending_mutex.unlock();
865		- const call = schedule_calls[index];
866		- schedule_calls[index] = null;
867		- return call;
868		-}
	866	+/// The deadline covers `addNotificationRequest:` only.
	867	+///
	868	+/// Stage one is `requestAuthorizationWithOptions:`, which puts a prompt on
	869	+/// someone's screen — timing that out would answer a question they are still
	870	+/// reading. Only the write afterwards is unattended, so the clock starts on
	871	+/// the grant rather than at dispatch, which is what #223 asks for.
	872	+const schedule_deadline_ms: u32 = 10_000;
869	873
870		-/// `void (^)(BOOL, NSError *)` — the authorization completion.
	874	+/// `void (^)(BOOL, NSError *)` — `requestAuthorizationWithOptions:`'s completion.
871	875	const BoolErrorBlock = extern struct {
872	876	isa: ?*anyopaque,
873	877	flags: c_int,
		⋯14 linesShow the 14 lines above this hunk@@ -888,17 +892,17 @@ const ErrorBlock = extern struct {
888	892	const bool_error_block_descriptor = BlockDescriptor{ .size = @sizeOf(BoolErrorBlock) };
889	893	const error_block_descriptor = BlockDescriptor{ .size = @sizeOf(ErrorBlock) };
890	894
891		-fn makeBoolErrorInvoke(comptime index: u5) *const anyopaque {
	895	+fn makeBoolErrorInvoke(comptime block: u5) *const anyopaque {
892	896	const S = struct {
893	897	fn invoke(_: *const BoolErrorBlock, granted: bool, _: Id) callconv(.c) void {
894		- authorizationFired(index, granted);
	898	+ authorizationFired(block, granted);
895	899	}
896	900	};
897	901	return @ptrCast(&S.invoke);
898	902	}
899	903
900		-fn makeBoolErrorBlocks() [ios_async.max_in_flight]BoolErrorBlock {
901		- var out: [ios_async.max_in_flight]BoolErrorBlock = undefined;
	904	+fn makeBoolErrorBlocks() [ios_pending.block_count]BoolErrorBlock {
	905	+ var out: [ios_pending.block_count]BoolErrorBlock = undefined;
902	906	for (&out, 0..) |*b, i| {
903	907	b.* = .{
904	908	.isa = &_NSConcreteGlobalBlock,
		⋯5 linesShow the 5 lines above this hunk@@ -910,17 +914,17 @@ fn makeBoolErrorBlocks() [ios_async.max_in_flight]BoolErrorBlock {
910	914	return out;
911	915	}
912	916
913		-fn makeErrorInvoke(comptime index: u5) *const anyopaque {
	917	+fn makeErrorInvoke(comptime block: u5) *const anyopaque {
914	918	const S = struct {
915	919	fn invoke(_: *const ErrorBlock, err: Id) callconv(.c) void {
916		- addFired(index, err);
	920	+ addFired(block, err);
917	921	}
918	922	};
919	923	return @ptrCast(&S.invoke);
920	924	}
921	925
922		-fn makeErrorBlocks() [ios_async.max_in_flight]ErrorBlock {
923		- var out: [ios_async.max_in_flight]ErrorBlock = undefined;
	926	+fn makeErrorBlocks() [ios_pending.block_count]ErrorBlock {
	927	+ var out: [ios_pending.block_count]ErrorBlock = undefined;
924	928	for (&out, 0..) |*b, i| {
925	929	b.* = .{
926	930	.isa = &_NSConcreteGlobalBlock,
		⋯5 linesShow the 5 lines above this hunk@@ -932,17 +936,17 @@ fn makeErrorBlocks() [ios_async.max_in_flight]ErrorBlock {
932	936	return out;
933	937	}
934	938
935		-var bool_error_blocks: [ios_async.max_in_flight]BoolErrorBlock =
	939	+var bool_error_blocks: [ios_pending.block_count]BoolErrorBlock =
936	940	if (is_darwin) makeBoolErrorBlocks() else undefined;
937		-var error_blocks: [ios_async.max_in_flight]ErrorBlock =
	941	+var error_blocks: [ios_pending.block_count]ErrorBlock =
938	942	if (is_darwin) makeErrorBlocks() else undefined;
939	943
940		-fn boolErrorBlock(ticket: ios_async.Ticket) *anyopaque {
941		- return @ptrCast(&bool_error_blocks[ticket.index]);
	944	+fn boolErrorBlock(block: u5) *anyopaque {
	945	+ return @ptrCast(&bool_error_blocks[block]);
942	946	}
943	947
944		-fn errorBlock(ticket: ios_async.Ticket) *anyopaque {
945		- return @ptrCast(&error_blocks[ticket.index]);
	948	+fn errorBlock(block: u5) *anyopaque {
	949	+ return @ptrCast(&error_blocks[block]);
946	950	}
947	951
948	952	/// Stage one, on whatever queue UserNotifications chose.
		⋯1 lineShow the 1 line above this hunk@@ -950,64 +954,120 @@ fn errorBlock(ticket: ios_async.Ticket) *anyopaque {
950	954	/// A refusal answers `PERMISSION_DENIED`, where the spec rejects with the
951	955	/// string "Permission denied" — the typed code every other migrated module
952	956	/// uses, carrying the same meaning in a form a page can branch on.
953		-fn authorizationFired(index: u5, granted: bool) void {
	957	+fn authorizationFired(block: u5, granted: bool) void {
954	958	if (!is_darwin) return;
955	959
956		- var call = takeScheduleCall(index) orelse {
957		- std.log.warn(
958		- "scheduleNotification authorization fired for slot {d} with no call recorded; ignored",
959		- .{index},
960		- );
961		- return;
	960	+ const entry = switch (schedule_calls.settle(block)) {
	961	+ .reply => |parked| parked,
	962	+ // Stage one arms no deadline, so `owed` can only mean the reply slot
	963	+ // was answered some other way. Nothing here to add.
	964	+ .late => return,
	965	+ .stray => {
	966	+ std.log.warn(
	967	+ "scheduleNotification authorization fired for block {d} with no call recorded; ignored",
	968	+ .{block},
	969	+ );
	970	+ return;
	971	+ },
962	972	};
	973	+ var call = entry.call;
	974	+ const ticket = entry.ticket;
963	975	const allocator = std.heap.c_allocator;
964	976
965	977	if (!granted) {
966	978	call.schedule.deinit(allocator);
967		- ios_async.deliverErrorCode(call.ticket, bridge_error.BridgeError.PermissionDenied);
	979	+ ios_async.deliverErrorCode(ticket, bridge_error.BridgeError.PermissionDenied);
968	980	return;
969	981	}
970	982
971	983	const request = buildRequest(&call.schedule, call.sels) catch |err| {
972	984	std.log.err("scheduleNotification could not build its request ({}); rejecting", .{err});
973	985	call.schedule.deinit(allocator);
974		- ios_async.deliverError(call.ticket);
	986	+ ios_async.deliverError(ticket);
975	987	return;
976	988	};
977	989
978	990	const center = notificationCenter() catch {
979	991	call.schedule.deinit(allocator);
980		- ios_async.deliverError(call.ticket);
	992	+ ios_async.deliverError(ticket);
	993	+ return;
	994	+ };
	995	+
	996	+ // A *fresh* block for stage two, claimed before the add for the reason the
	997	+ // first claim exists: the completion can fire before `msgSend` returns.
	998	+ // Fresh rather than the same one, because stage one's block was settled
	999	+ // above and may already belong to another call.
	1000	+ const add_block = schedule_calls.claim(ticket, call) orelse {
	1001	+ std.log.warn(
	1002	+ "scheduleNotification: all {d} completion blocks are still owed a completion " ++
	1003	+ "UserNotifications never delivered; the request was built but not filed",
	1004	+ .{ios_pending.block_count},
	1005	+ );
	1006	+ call.schedule.deinit(allocator);
	1007	+ ios_async.deliverError(ticket);
981	1008	return;
982	1009	};
983	1010
984		- // Republished before the add, for the reason the first publish exists: the
985		- // completion can fire before `msgSend` returns.
986		- publishScheduleCall(call.ticket, call.sels, call.schedule);
	1011	+ // The clock starts here (#223): the prompt above had a person in front of
	1012	+ // it, and only this write is unattended.
	1013	+ ios_async.scheduleDeadline(ticket, schedule_deadline_ms, scheduleTimedOut);
987	1014
988	1015	const Fn = *const fn (Id, Id, Id, *anyopaque) callconv(.c) void;
989	1016	const func: Fn = @ptrCast(&objc.objc_msgSend);
990		- func(center, call.sels.add_request, request, errorBlock(call.ticket));
	1017	+ func(center, call.sels.add_request, request, errorBlock(add_block));
	1018	+}
	1019	+
	1020	+/// On the main queue, `schedule_deadline_ms` after the add. Does nothing when
	1021	+/// the completion got there first, which is the usual case: the timer cannot
	1022	+/// be cancelled, so it always fires.
	1023	+fn scheduleTimedOut(context: ?*anyopaque) callconv(.c) void {
	1024	+ if (!is_darwin) return;
	1025	+
	1026	+ var entry = schedule_calls.expireIfWaiting(ios_async.ticketFromDeadline(context)) orelse return;
	1027	+ // The schedule is this call's to free once the deadline has taken it: the
	1028	+ // completion that eventually fires will find the block owed and stop.
	1029	+ entry.call.schedule.deinit(std.heap.c_allocator);
	1030	+ std.log.warn(
	1031	+ "scheduleNotification: addNotificationRequest:withCompletionHandler: did not call its " ++
	1032	+ "completion within {d} ms; answering TIMEOUT",
	1033	+ .{schedule_deadline_ms},
	1034	+ );
	1035	+ ios_async.deliverErrorCode(entry.ticket, bridge_error.BridgeError.Timeout);
991	1036	}
992	1037
993	1038	/// Stage two. Answers the identifier the request was filed under, which is what
994	1039	/// the page needs to cancel it later.
995		-fn addFired(index: u5, err: Id) void {
	1040	+fn addFired(block: u5, err: Id) void {
996	1041	if (!is_darwin) return;
997	1042
998		- var call = takeScheduleCall(index) orelse {
999		- std.log.warn(
1000		- "scheduleNotification add fired for slot {d} with no call recorded; ignored",
1001		- .{index},
1002		- );
1003		- return;
	1043	+ const entry = switch (schedule_calls.settle(block)) {
	1044	+ .reply => |parked| parked,
	1045	+ .late => |started_ms| {
	1046	+ // The deadline answered and freed the schedule; this completion
	1047	+ // owns nothing and must not touch it.
	1048	+ std.log.warn(
	1049	+ "scheduleNotification: an add completion arrived {d} ms after the call, after " ++
	1050	+ "its deadline had answered; ignored",
	1051	+ .{compat.milliTimestamp() - started_ms},
	1052	+ );
	1053	+ return;
	1054	+ },
	1055	+ .stray => {
	1056	+ std.log.warn(
	1057	+ "scheduleNotification add fired for block {d} with no call recorded; ignored",
	1058	+ .{block},
	1059	+ );
	1060	+ return;
	1061	+ },
1004	1062	};
	1063	+ var call = entry.call;
	1064	+ const ticket = entry.ticket;
1005	1065	const allocator = std.heap.c_allocator;
1006	1066	defer call.schedule.deinit(allocator);
1007	1067
1008	1068	if (err != null) {
1009	1069	std.log.warn("scheduleNotification: addNotificationRequest reported an error", .{});
1010		- ios_async.deliverErrorCode(call.ticket, bridge_error.BridgeError.NativeCallFailed);
	1070	+ ios_async.deliverErrorCode(ticket, bridge_error.BridgeError.NativeCallFailed);
1011	1071	return;
1012	1072	}
1013	1073
		⋯2 linesShow the 2 lines above this hunk@@ -1016,7 +1076,7 @@ fn addFired(index: u5, err: Id) void {
1016	1076	// here means the request was filed under an identifier nothing kept.
1017	1077	// Answering anyway would hand the page a value it cannot cancel with.
1018	1078	std.log.err("scheduleNotification: the request was added with no identifier recorded", .{});
1019		- ios_async.deliverError(call.ticket);
	1079	+ ios_async.deliverError(ticket);
1020	1080	return;
1021	1081	};
1022	1082
		⋯1 lineShow the 1 line above this hunk@@ -1024,12 +1084,12 @@ fn addFired(index: u5, err: Id) void {
1024	1084	// page-supplied identifier can contain a quote.
1025	1085	var out: std.ArrayListUnmanaged(u8) = .empty;
1026	1086	defer out.deinit(allocator);
1027		- out.append(allocator, '"') catch return ios_async.deliverError(call.ticket);
	1087	+ out.append(allocator, '"') catch return ios_async.deliverError(ticket);
1028	1088	bridge_error.appendJsonEscaped(allocator, &out, id) catch
1029		- return ios_async.deliverError(call.ticket);
1030		- out.append(allocator, '"') catch return ios_async.deliverError(call.ticket);
	1089	+ return ios_async.deliverError(ticket);
	1090	+ out.append(allocator, '"') catch return ios_async.deliverError(ticket);
1031	1091
1032		- ios_async.deliverJson(call.ticket, out.items);
	1092	+ ios_async.deliverJson(ticket, out.items);
1033	1093	}
1034	1094
1035	1095	/// Build `UNNotificationRequest` from the parsed schedule.
		⋯312 linesShow the 312 lines above this hunk@@ -1348,14 +1408,77 @@ test "each schedule slot has its own block, and a call is taken once" {
1348	1408	try testing.expect(bool_error_blocks[0].invoke != bool_error_blocks[1].invoke);
1349	1409	try testing.expect(error_blocks[0].invoke != error_blocks[1].invoke);
1350	1410
1351		- // Take-and-clear is what makes a double-fired completion a no-op rather
	1411	+ // Settle-and-free is what makes a double-fired completion a no-op rather
1352	1412	// than a second reply.
1353	1413	const allocator = testing.allocator;
1354	1414	var schedule = try parseSchedule(allocator, "{\"notification\":{\"title\":\"t\"}}");
1355	1415	defer schedule.deinit(allocator);
1356		- publishScheduleCall(.{ .index = 2, .generation = 9 }, undefined, schedule);
1357		- try testing.expect(takeScheduleCall(2) != null);
1358		- try testing.expect(takeScheduleCall(2) == null);
	1416	+ const ticket: ios_async.Ticket = .{ .index = 2, .generation = 9 };
	1417	+ const block = schedule_calls.claim(ticket, .{ .sels = undefined, .schedule = schedule }) orelse
	1418	+ return error.NoFreeBlock;
	1419	+ try testing.expect(schedule_calls.settle(block) == .reply);
	1420	+ try testing.expect(schedule_calls.settle(block) == .stray);
	1421	+}
	1422	+
	1423	+test "the add's deadline answers once, and the completion it beat is ignored" {
	1424	+ // #223: stage two is `addNotificationRequest:`, which no person is waiting
	1425	+ // in front of. Without the table a deadline would free the reply slot, the
	1426	+ // next schedule would lease it and get the same block, and this late
	1427	+ // completion would answer that call.
	1428	+ const allocator = testing.allocator;
	1429	+ var schedule = try parseSchedule(allocator, "{\"notification\":{\"title\":\"t\"}}");
	1430	+ defer schedule.deinit(allocator);
	1431	+
	1432	+ const ticket: ios_async.Ticket = .{ .index = 4, .generation = 21 };
	1433	+ const block = schedule_calls.claim(ticket, .{ .sels = undefined, .schedule = schedule }) orelse
	1434	+ return error.NoFreeBlock;
	1435	+
	1436	+ const expired = schedule_calls.expireIfWaiting(ticket) orelse return error.DeadlineFoundNothing;
	1437	+ try testing.expectEqual(@as(u32, 21), expired.ticket.generation);
	1438	+
	1439	+ // Owed, not free: the next schedule must not be given this block.
	1440	+ const next: ios_async.Ticket = .{ .index = 4, .generation = 22 };
	1441	+ var other = try parseSchedule(allocator, "{\"notification\":{\"title\":\"u\"}}");
	1442	+ defer other.deinit(allocator);
	1443	+ const elsewhere = schedule_calls.claim(next, .{ .sels = undefined, .schedule = other }) orelse
	1444	+ return error.NoFreeBlock;
	1445	+ try testing.expect(elsewhere != block);
	1446	+
	1447	+ try testing.expect(schedule_calls.settle(block) == .late);
	1448	+ _ = schedule_calls.settle(elsewhere);
	1449	+}
	1450	+
	1451	+test "the authorization prompt is not on a clock, but the write after it is" {
	1452	+ // Stage one puts a prompt on someone's screen; timing that out would
	1453	+ // answer a question they are still reading. Only the add is unattended,
	1454	+ // which is why the deadline is armed in `authorizationFired` rather than
	1455	+ // in the dispatch above it.
	1456	+ const source = @embedFile("bridge_mobile_notifications.zig");
	1457	+
	1458	+ // Split, and joined at comptime. Written whole, the needle would appear in
	1459	+ // this test's own source and `indexOf` would match *that* — the assertion
	1460	+ // below then passes with the real call deleted, which is exactly what it
	1461	+ // did before this comment existed.
	1462	+ const needle = "scheduleDeadline(ticket, " ++ "schedule_deadline_ms";
	1463	+ var found: usize = 0;
	1464	+ var armed_at: usize = 0;
	1465	+ var search: usize = 0;
	1466	+ while (std.mem.indexOfPos(u8, source, search, needle)) |at| {
	1467	+ found += 1;
	1468	+ armed_at = at;
	1469	+ search = at + needle.len;
	1470	+ }
	1471	+ // Exactly one: a second would mean the needle matched something other than
	1472	+ // the call, and the ordering below would be about the wrong thing.
	1473	+ try testing.expectEqual(@as(usize, 1), found);
	1474	+
	1475	+ const dispatch_at = std.mem.indexOf(u8, source, "un_options_alert_sound_badge") orelse
	1476	+ return error.DispatchNotFound;
	1477	+ try testing.expect(armed_at > dispatch_at);
	1478	+
	1479	+ // And well under the 30s `_invoke` gives up after, so the caller hears
	1480	+ // this answer rather than its own timeout.
	1481	+ try testing.expect(schedule_deadline_ms < 30_000);
1359	1482	}
1360	1483
1361	1484	test "the payload is ignored, not parsed" {