ReviewOS

also looking at this

craft-native/craft

fix(ios): give getHealthData and saveHealthWorkout deadlines, on the shared block table

#250
Merged glennmichael123 wants to merge fix/ios-health-deadlines into main
1 file +343 -122
9 days ago

Refs #223 — actions three and four of six: getHealthData and saveHealthWorkout. Written under #249's per-claim ratchet, which this file is the reason for.

Three actions, one table

bridge_mobile_health.zig had three actions sharing one side table keyed by reply-slot index — not the two #223 lists:

actionwho answersdeadline
getHealthDataHKStatisticsQuery15 s
saveHealthWorkoutthree chained healthd completions8 s per stage
requestHealthAuthorizationa person, at the HealthKit sheetnone, declared

All three now park on ios_pending.Table, and all five block arrays are indexed per block rather than per slot. That third row is the whole reason #249 had to land first: the old per-module ratchet would have passed this file regardless of which action lacked a deadline, because the file calls scheduleDeadline either way. Under the per-claim rule, requestHealthAuthorization has to say so:

// no-deadline: a person answers the HealthKit authorization sheet, and
// timing that out would answer a question they are still reading.

Why every workout stage re-arms its own deadline

The obvious design is one deadline, armed at dispatch, covering all three stages. It has a hole.

Between one stage settling its block and the next claiming a fresh one, no block holds the ticket. A deadline that fires in that moment calls expireIfWaiting, finds nothing, and does nothing — and timers fire once. The next stage then runs with no clock at all, which is exactly the hang #223 exists to remove.

So each stage that claims a block also arms a deadline. An earlier one landing between stages no-ops; the next stage already has its own. Three stages at 8 s stays under _invoke's 30 s even if every earlier deadline were to land in such a gap. A test drives that exact sequence — settle, a deadline finding nothing, claim, a deadline finding stage two.

Fallible steps above the lease

#223 lists these, and they are all fixed:

  • requestHealthAuthorization and saveHealthWorkout looked up their selector after publishing the call.
  • getHealthData resolved its class and three selectors after publishCall. They now resolve before acquire. What remains between the claim and the framework is alloc and the init, and an errdefer releases the block if either were to fail — it was never handed to anything that would call it back.
  • Two chain stages had sel_registerName(...) orelse return; after republishing. That parked the call with no framework call made: a promise nothing could settle, deadline or not. Each stage now resolves its next selector before claiming, and a failure answers the call.

That answer is a rejection, with the saved workout's id logged. The workout really was written, but its route will not be, and resolving would look identical to a finish that succeeded with no route — the reasoning routeInserted already documents for its own failure path. routeAbandoned carries it for all four such exits.

Tests, and one of mine that was right for the wrong reason

mutationcaught by
stage two does not re-armthe per-claim ratchet, naming fn workoutSaved
the authorization sheet's no-deadline: removedthe per-claim ratchet, naming fn requestAuthorization
getHealthData loses its deadlinethe per-claim ratchet, naming fn getHealthData
stage two re-armed with statsTimedOutthe one-arm-per-stage test

The last row is worth explaining, because I got it wrong first. I wrote the one-arm-per-stage test with a comment saying a dispatch-only deadline "would be satisfied by the ratchet and still strand stage two". That was true of the old ratchet and false of #249's — the first mutation above proves it. So I checked what the test actually adds, and found something better: re-arming stage two with the wrong handler passes the ratchet, which asks only for some deadline, and statsTimedOut answers TIMEOUT without freeing the workout chain — a leak on every timed-out workout. The handler is now part of the test's needle, and its comment says that instead.

The needle is joined at comptime, so it cannot match the test's own text — the self-match that let #248's first ordering test pass with its call deleted.

Verification

zig build test:ios (on the rebased branch) and zig build test:android, Pickier (0 errors), the ios/android package tests (154), and the mutations above.

Left on #223 after this: openURL and openSettings, both of which put something in front of a person — a confirmation, or Settings itself — and need that decided before any deadline is written.

1 changed file on the files tab, with 0 review threads.

Sign in to comment on this pull request.