Fetch a path's metadata over REST when the delta stream never carries it - #579
Conversation
Code review — 7 personas, findings appliedReviewers: correctness, testing, maintainability, project-standards (always-on) + security (URL built from stream-supplied path segments), reliability (outbound request on the delta hot path, swallowed errors), adversarial (59 changed lines in the central data hub). The finding that matteredThe premise in the original PR description was wrong, and three reviewers caught it independently. I claimed the server's meta hook reads only the static
The consequence for this PR was serious: a 404 means "not yet" as often as "never", and the original code spent its single attempt on it — reproducing the exact defect it set out to fix, in Skip. Fixed: the attempt is now retried on later values up to a ceiling of 3. One P1 raised and refutedThe adversarial reviewer argued (adv-1, confidence 0.72) that The value endpoint also returns Applied
Tests went from 5 to 13 in the backfill block, covering the retry ceiling, retry-then-success, encoding, null endpoint mid-session, no-trailing-slash root, unusable bodies, the mid-flight delta race, and both new reachability paths. Suite: 2043 passing. Not applied
|
39952bd to
edc5a56
Compare
Code review — 7 personas, findings appliedReviewers: correctness, testing, maintainability, project-standards, reliability, api-contract, adversarial. Each ran in a fresh context against an isolated worktree. The premise holds; the P1 against it is refutedThe api-contract reviewer traced Tested against a HaLOS device (signalk-server 2.30.0) with a course active: The v1 model does carry course-provider meta on 2.30.0. The source reading is incomplete. The adversarial reviewer independently reached the same conclusion from the server source: it is the same The retry design did not survive measurementThree reviewers, two with scratch specs run against the branch, found the same defect from different angles. Measured:
The counter incremented at issue time with nothing tracking an outstanding request, so the budget measured deltas rather than answers. The doc comment's claim that "the next value delta supplies both the retry and its spacing" holds only when the delta interval exceeds the round trip — false for any 5–10 Hz path. Reworked: one request per path outstanding, attempt counted on completion. Three attempts now mean three answers. The budget also never re-armed — its only reset lived in Four reviewers on the same forged-context hole
Also applied
TestsFour cases added, two mutation-verified individually: dropping the in-flight guard fails "keeps one request per path outstanding"; narrowing the self-context guard to reject any stated context fails "fetches for a delta whose context is the vessel's own self URN". That second one matters — every prior case used Not actionedPercent-encoded segments are a permanent 404 (verified live: Gate on the head commit: 2054 tests pass, |
6358cf2 to
59184c8
Compare
The server can drop a path's metadata for a whole connection. Its WebSocket hook resolves meta from a registry that gains entries at runtime, but marks a path as sent before checking whether the lookup found anything, so a producer that emits a value before registering its meta costs that connection the metadata. The course calculations do exactly that, and a page reload was the only cure. REST re-reads the registry per request, so it can still answer. A displayed self path that produces a value with no meta cached now triggers a fetch. The request is subject to the same race, so it retries — but by answers, not by deltas. One request per path is outstanding at a time and the attempt is counted on completion, because counting at issue time let a 10 Hz path spend all three inside 300 ms on concurrent requests asking the same unanswered question, and a stalled server then killed the whole budget on one shared deadline. The budget re-arms when endpoint discovery re-runs, which is both a fresh chance at the metadata and the moment the stream most often loses it. Bounded elsewhere too: self contexts only, recorded from what the delta states because the cache key cannot be inverted back to it; paths something displays; a 5 s timeout; and a response accepted only when it carries a recognisable metadata key, since anything cached here is permanent for the page. The call runs after the delta's registration fan-out so that nothing it does can cost a widget its value.
59184c8 to
11473a7
Compare
Why
A path that materialises after Skip is already connected can lose its metadata for the whole session. The Signal K server's WebSocket hook resolves meta from a registry that gains entries at runtime, but marks a path as sent before checking whether the lookup found anything — so a producer that emits a value before registering the path's meta costs that connection its one chance. The course calculations do exactly this: they exist only while a course is active, and
@signalk/course-providersends values first, meta second.The user sees a Numeric widget on the bearing to the next waypoint reading radians instead of degrees, with a unit menu offering every conversion group rather than the path's own. A page reload fixes it, because
spark.sentMetaDatastarts empty again.How
REST re-reads the same registry per request, so it can still answer after the WebSocket has given up. When a displayed self path produces a value with no metadata cached, Skip fetches
vessels/self/<path>/meta.Verified on a HaLOS device (signalk-server 2.30.0) with a course active: that endpoint returns 200 with
units: "rad"and fulldisplayUnits, for the exact path the issue names.The request is subject to the same race — too early and the server has nothing either — so it retries rather than spending its one chance. What bounds it:
vessels/self. Self-ness is recorded where the delta states it, because the cache key cannot be inverted back to it: a foreign context namedself.<something>produces a key indistinguishable from a self path's.Tests
18 cases in
data.service.spec.ts. Reverting the delta-time call site fails 9; reverting the registration call site fails 1. Mutation-checked individually: dropping the in-flight guard, and narrowing the self-context guard to reject any delta that states its context, each fail exactly one named test.Not addressed here
The server-side defect is the root cause and is tracked separately.
Fixes #545