Repository navigation
feat(statistics): require Cloud terms and allow local contribution - #32
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff2eb5ccea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if c.u.Statistics && !statisticsAccess(r) { | ||
| c.handle.DropCopy() | ||
| c.u.store.Forget(c.home) | ||
| return fmt.Errorf("%w: updated Cloud terms including statistics are required", errProtocol) |
There was a problem hiding this comment.
When statistics collection is enabled, every valid withdrawn roster fails statisticsAccess, so this branch returns before dropCopy and the later registry update run. As a result, DELETE /api/cloud clears in-memory readings but does not prune extension tokens, persist the withdrawal, or send the signed purged receipt; the box will keep retrying against the old stored grant. Process the withdrawn shape through the existing withdrawal path before rejecting rosters that lack the new terms.
Useful? React with 👍 / 👎.
| on := g.Controller == cloudwire.ControllerSourceful && g.StatsPolicy == cloudwire.StatsPolicy && | ||
| len(g.StatsID) == 16 && g.StatsSinceMs > 0 && slices.Contains(g.Purposes, protocol.CloudPurposeStats) && | ||
| slices.Contains(g.Purposes, protocol.CloudPurposeServe) | ||
| len(g.StatsID) == 16 && g.StatsSinceMs > 0 && slices.Contains(g.Purposes, protocol.CloudPurposeStats) |
There was a problem hiding this comment.
If a signed roster contains the current statistics purpose, ID, policy, and timestamp but lacks the required terms or includes metadata forbidden for a statistics-only roster, this predicate still creates a statistics grant before statisticsAccess rejects the roster. The box can then reconnect and send stats directly, because report ingestion checks this database grant rather than requiring an accepted roster. Only enable the grant after the full roster shape and terms have been accepted, while retaining a separate clearing path for withdrawals.
Useful? React with 👍 / 👎.
| // statisticsOnly exposes the signed upload path and no remote recipient. | ||
| func statisticsOnly(r cloudwire.Roster) bool { | ||
| g := r.Grant | ||
| return len(g.ID) == cloudwire.GrantIDBytes && g.Controller == cloudwire.ControllerSourceful && len(g.Scopes) == 0 && len(g.Purposes) == 1 && g.Purposes[0] == protocol.CloudPurposeStats && g.StatsPolicy == cloudwire.StatsPolicy && g.TermsPolicy == cloudwire.StatsPolicy && len(g.StatsID) == 16 && len(r.Devices) == 0 && len(r.Pending) == 0 && len(r.Noise) == 0 && len(r.WebPush) == 0 |
There was a problem hiding this comment.
A statistics-only roster with StatsSinceMs <= 0 passes this predicate and is stored as an accepted local contribution, but StatisticsGrant treats that same grant as disabled, so all subsequent reports are silently ignored. Include the positive timestamp requirement here so an accepted statistics-only roster cannot advertise an inert contribution.
Useful? React with 👍 / 👎.
1 participant
Statistics are part of the new FTW Cloud terms for every Cloud home. Local owners can instead contribute statistics without remote access. This follows Fredrik's 17 October product decision and pairs with srcfl/ftw#1700, srcfl/ftw-app#87 and srcfl/ftw-web#23.
Accept only current Cloud terms when collection is approved, including after restoring cached rosters at restart. A statistics-only roster has no phone keys, pairing codes, Noise key, WebPush, live cache or access scopes. Signed withdrawal removes unpublished rows, persists the withdrawal and sends the signed deletion receipt before Cloud closes access. Rejected rosters cannot create a statistics grant. Every connection must present an accepted current roster before sending reports, including when an earlier grant remains on disk. A local-only grant needs a positive start time. Ignore older roster revisions before changing a statistics grant.
Collection remains off by default and in production. Nothing here signs the C8 DPIA or establishes a lawful basis for requiring statistics within Cloud. The updated review draft records both flows, conditional-consent concerns, retention, public-cell risks, infrastructure checks and rights handling that still need evidence. Accepting terms alone does not settle those questions. Deploy compatible Core and clients and review existing-home migration before any policy activation.
Validation: vet, full race suite against the published Core protocol pin, full dev-tag suite and import-boundary checks pass. The initial runs exposed ordering assumptions in QUIC fixtures: a later stream frame could overtake the datagram the test expected first. Replace the 50/100 ms sleeps with observed heartbeat/live barriers and wait for the initial grant/table before the overtaking-datagram test. Both affected tests pass ten race-enabled repeats. Product code and deadlines are unchanged. Regressions cover signed local-only ingestion without phone access, metadata rejection, legacy-term rejection on restart, normal withdrawal and paused deletion. No raw household API was added. Non-test changes in uplink are about 30 lines; registry removes the readable-copy prerequisite and store clarifies why cached access can close.