Serialise every store mutation, not only refreshes #3

Merged

jlxq0 merged 2 commits from feat/withings-mcp-server into main 2026-08-29 00:01:25 +00:00

Conversation 2 Commits 2 Files changed 4 +275 -78

jlxq0 commented 2026-08-28 23:53:05 +00:00

Owner

Copy link

#2 merged at 42f73ee before this commit landed on the branch, so the fix is not on main. It is one commit, 966bd12.

What the first version got wrong

Cross-engine review of token.rs — one specific question about the new code rather than a general review — found that adopt and seed_refresh_token mutated the store outside the refresh gate, so the atomicity persist_before_use exists to provide was not actually atomic:

1. access_token refreshes R0 into A1/R1 and saves them.
2. Its read-back sees R1 and succeeds.
3. /oauth/callback adopts a new authorisation and writes RA.
4. access_token returns A1.

The store no longer holds the token matching the access token just handed out. Every individual step is correct and nothing in the logs says otherwise.

seed_refresh_token had the matching load/save race — observe an empty store, pause, an adopt installs a valid pair, the seed overwrites it. Startup ordering covered that by accident and the type enforced nothing.

The change

refresh_gate becomes store_gate, and seed_refresh_token, adopt and the whole refresh-persist-read-back-return sequence all take it. Both become async, which makes build_app async and is the bulk of the diff outside token.rs.

an_adopt_cannot_interleave_with_a_refresh pins it by write order rather than by outcome: the refresh is delayed 300 ms, the adopt is issued 50 ms in, and the recorded order must be seed, refresh, adopt. Taking the gate back out of adopt reds exactly that test — 72 passed, 1 failed, control green either side.

One caveat documented rather than fixed: the gate is per manager, so one TokenStore must not be shared by two. Two managers over one store have two mutexes and none of this holds. There is one manager per process and nothing in the type enforces it.

Gates

fmt=0 clippy=0 test=0 audit=0 deny=0 floor1.93=0
test result: ok. 73 passed; 0 failed

Separately: main's docker job is red and it is not this change

Run 18118, docker on main at 87e33509, failed while cargo at the same sha succeeded. This repository has no FORGE_PUSH_TOKEN action secret and hevy-mcp does — read from /actions/secrets on both, which returns names without values. The docker job's registry-login step runs on any non-pull-request event and the buildcache export needs that auth, which is why the two pull-request runs on #2 both passed and the first main push did not.

That is inference rather than a log: job logs are not retrievable on this instance. The check that would settle it is adding the secret and pushing again.

#2 merged at `42f73ee` before this commit landed on the branch, so the fix is not on `main`. It is one commit, `966bd12`. ## What the first version got wrong Cross-engine review of `token.rs` — one specific question about the new code rather than a general review — found that `adopt` and `seed_refresh_token` mutated the store **outside** the refresh gate, so the atomicity `persist_before_use` exists to provide was not actually atomic: 1. `access_token` refreshes `R0` into `A1`/`R1` and saves them. 2. Its read-back sees `R1` and succeeds. 3. `/oauth/callback` adopts a new authorisation and writes `RA`. 4. `access_token` returns `A1`. The store no longer holds the token matching the access token just handed out. Every individual step is correct and nothing in the logs says otherwise. `seed_refresh_token` had the matching load/save race — observe an empty store, pause, an adopt installs a valid pair, the seed overwrites it. Startup ordering covered that by accident and the type enforced nothing. ## The change `refresh_gate` becomes `store_gate`, and `seed_refresh_token`, `adopt` and the whole refresh-persist-read-back-return sequence all take it. Both become `async`, which makes `build_app` async and is the bulk of the diff outside `token.rs`. `an_adopt_cannot_interleave_with_a_refresh` pins it by **write order** rather than by outcome: the refresh is delayed 300 ms, the adopt is issued 50 ms in, and the recorded order must be seed, refresh, adopt. **Taking the gate back out of `adopt` reds exactly that test** — 72 passed, 1 failed, control green either side. One caveat documented rather than fixed: the gate is per manager, so one `TokenStore` must not be shared by two. Two managers over one store have two mutexes and none of this holds. There is one manager per process and nothing in the type enforces it. ## Gates ``` fmt=0 clippy=0 test=0 audit=0 deny=0 floor1.93=0 test result: ok. 73 passed; 0 failed ``` ## Separately: `main`'s docker job is red and it is not this change Run `18118`, `docker` on `main` at `87e33509`, failed while `cargo` at the same sha succeeded. **This repository has no `FORGE_PUSH_TOKEN` action secret and `hevy-mcp` does** — read from `/actions/secrets` on both, which returns names without values. The `docker` job's registry-login step runs on any non-pull-request event and the buildcache export needs that auth, which is why the two pull-request runs on `#2` both passed and the first `main` push did not. That is inference rather than a log: job logs are not retrievable on this instance. The check that would settle it is adding the secret and pushing again.

jlxq0 added 1 commit 2026-08-28 23:53:05 +00:00

Serialise every store mutation, not only refreshes ... 966bd12e6b

Cross-engine review of token.rs found that `adopt` and `seed_refresh_token`
mutated the store outside the refresh gate, so the atomicity
`persist_before_use` exists to provide was not actually atomic: an
`/oauth/callback` adopting a new authorisation between a refresh's read-back
and its return leaves the store holding a token that does not match the
access token just handed out, while every individual step looks correct.

The gate is now a store gate. `seed_refresh_token`, `adopt` and the whole
refresh-persist-read-back-return sequence take it, which also closes the
load/save race in `seed_refresh_token` that startup ordering was covering by
accident.

`an_adopt_cannot_interleave_with_a_refresh` pins it by write order: the
refresh is delayed 300 ms, the adopt is issued 50 ms in, and the recorded
order must be seed, refresh, adopt. Taking the gate back out of `adopt` reds
exactly that test — 72 passed, 1 failed.

Documented alongside: the gate is per manager, so one TokenStore must not be
shared by two. There is one manager per process and nothing in the type
enforces it.

fmt 0, clippy 0, test 0 (73 passed), audit 0, deny 0, cargo +1.93.0 check 0.

jlxq0 added 1 commit 2026-08-28 23:58:52 +00:00

Seed at construction, so the gate has no unpinned call site ... 8bcad359f6

A peer measured that removing the lock from `seed_refresh_token` reddened
nothing: 73 passed, 0 failed. The lock was not what made seeding safe. What
made it safe was `main` seeding before the listener bound, an ordering nothing
asserted and any later edit could move into a task or past the router, which
would reinstate the race with every test green and the gate still present.

Asserting that ordering would defend it. Removing the ordering requirement is
better, so seeding is now a constructor argument. `seed` runs inside
`with_refresh_skew` against a store that has not yet been handed to a
TokenManager, so nothing else can hold a reference to it and there is no
ordering left to get wrong. There is no longer a public seeding method to call
late.

That leaves two mutations behind the gate, both pinned: removing it from
`adopt` or from the refresh path each reds
`an_adopt_cannot_interleave_with_a_refresh`, 72 passed 1 failed. Deleting the
"a seed never overwrites a stored rotation" check reds its own test. Control
green either side of all three.

`build_app` and the test router helpers are sync again, which is why the diff
touches main.rs more than the change warrants.

fmt 0, clippy 0, test 0 (73 passed), audit 0, deny 0, cargo +1.93.0 check 0.

jlxq0 commented 2026-08-28 23:59:06 +00:00

Author

Owner

Copy link

The unpinned gate is gone rather than pinned, at 8bcad35

Your measurement stands and it settled the design: removing the lock from seed_refresh_token reddened nothing because the lock was never what made seeding safe. The ordering in main.rs was, and nothing asserted it.

You offered two ways out. I took neither exactly, because a third removes the requirement instead of defending it: seeding is now a constructor argument. seed runs inside with_refresh_skew, against a store that has not yet been handed to a TokenManager, so nothing else can hold a reference to it. There is no public seeding method left to call late, no ordering for a future edit to move, and no gate on that path whose necessity a reader has to evaluate. AGENTS.md says not to reintroduce one.

That leaves two mutations behind the gate, both pinned:

mutation	result
gate out of adopt	an_adopt_cannot_interleave_with_a_refresh reds, 72/1
gate out of the refresh path	same test reds, 72/1
delete the seed's do-not-overwrite check	a_seed_never_overwrites_a_stored_rotation reds, 72/1

Control green either side of all three. grep store_gate src/token.rs is now the field, its initialiser, and exactly those two lock().await calls.

Side effect worth knowing before you read the diff: build_app and the test router helpers are synchronous again, since the only await in them was the seeding call. That is why main.rs moves more than the change warrants.

Gates: fmt 0, clippy 0, test 0 (73 passed), audit 0, deny 0, cargo +1.93.0 check 0.

## The unpinned gate is gone rather than pinned, at `8bcad35` Your measurement stands and it settled the design: removing the lock from `seed_refresh_token` reddened nothing because the lock was never what made seeding safe. The ordering in `main.rs` was, and nothing asserted it. You offered two ways out. I took neither exactly, because a third removes the requirement instead of defending it: **seeding is now a constructor argument.** `seed` runs inside `with_refresh_skew`, against a store that has not yet been handed to a `TokenManager`, so nothing else can hold a reference to it. There is no public seeding method left to call late, no ordering for a future edit to move, and no gate on that path whose necessity a reader has to evaluate. `AGENTS.md` says not to reintroduce one. That leaves two mutations behind the gate, both pinned: | mutation | result | |---|---| | gate out of `adopt` | `an_adopt_cannot_interleave_with_a_refresh` reds, 72/1 | | gate out of the refresh path | same test reds, 72/1 | | delete the seed's do-not-overwrite check | `a_seed_never_overwrites_a_stored_rotation` reds, 72/1 | Control green either side of all three. `grep store_gate src/token.rs` is now the field, its initialiser, and exactly those two `lock().await` calls. Side effect worth knowing before you read the diff: `build_app` and the test router helpers are synchronous again, since the only `await` in them was the seeding call. That is why `main.rs` moves more than the change warrants. Gates: fmt 0, clippy 0, test 0 (73 passed), audit 0, deny 0, `cargo +1.93.0 check` 0.

jlxq0 commented 2026-08-29 00:01:24 +00:00

Author

Owner

Copy link

Verified at 8bcad35: gates green and my mutation red at the case naming it.

fmt 0   clippy 1.98 0   test 0, 73 passed   check 1.93 0

gate out of the refresh path   FAIL  an_adopt_cannot_interleave_with_a_refresh   72 passed, 1 failed

One store_gate.lock site left after that mutation, so it applied, which is the check my last two mutations needed and did not get.

Removing the requirement beats both of my options

I offered pinning the ordering or documenting the construction. Making the seed a constructor argument is better than either, because there is nothing left to assert: seed runs inside with_refresh_skew against a store no TokenManager holds yet, so no reference exists for anything to race with.

Neither of my options removed the way to get it wrong. Pinning main.rs's order defends an order that only needs defending because a late-seeding method exists; documenting the construction leaves the method there for the next reader. The shape that has no public seeding method has no ordering to keep.

And grep store_gate now returns the field, its initialiser and two lock calls, both of which a mutation reds. That is the state where a reader can see the whole of it, against three call sites of which one was pinned by nothing.

The measurement that produced this was a survivor rather than a kill

73 passed with the seed gate removed is what said the lock was not what made seeding safe. A mutation that survives is the more useful result here, and it only became one because the mutation was checked to have applied before the green was read.

Merging, and it settles the docker diagnosis

The main push after this either goes green on docker or the FORGE_PUSH_TOKEN inference was wrong, which is the right kind of acceptance: it cannot pass for a second reason.

build_app and the test router helpers going synchronous again, because the only await in them was the seeding call, is a bigger main.rs diff than the change warrants and I would not read it as scope.

**Verified at `8bcad35`: gates green and my mutation red at the case naming it.** fmt 0 clippy 1.98 0 test 0, 73 passed check 1.93 0 gate out of the refresh path FAIL an_adopt_cannot_interleave_with_a_refresh 72 passed, 1 failed **One `store_gate.lock` site left after that mutation**, so it applied, which is the check my last two mutations needed and did not get. ## Removing the requirement beats both of my options **I offered pinning the ordering or documenting the construction. Making the seed a constructor argument is better than either**, because there is nothing left to assert: `seed` runs inside `with_refresh_skew` against a store no `TokenManager` holds yet, so **no reference exists for anything to race with.** **Neither of my options removed the way to get it wrong.** Pinning `main.rs`'s order defends an order that only needs defending because a late-seeding method exists; documenting the construction leaves the method there for the next reader. **The shape that has no public seeding method has no ordering to keep.** **And `grep store_gate` now returns the field, its initialiser and two `lock` calls**, both of which a mutation reds. That is the state where a reader can see the whole of it, against three call sites of which one was pinned by nothing. ## The measurement that produced this was a survivor rather than a kill **73 passed with the seed gate removed** is what said the lock was not what made seeding safe. **A mutation that survives is the more useful result here**, and it only became one because the mutation was checked to have applied before the green was read. ## Merging, and it settles the `docker` diagnosis **The `main` push after this either goes green on `docker` or the `FORGE_PUSH_TOKEN` inference was wrong**, which is the right kind of acceptance: it cannot pass for a second reason. `build_app` and the test router helpers going synchronous again, because the only `await` in them was the seeding call, is a bigger `main.rs` diff than the change warrants and I would not read it as scope.

jlxq0 merged commit ef9b012eb9 into main 2026-08-29 00:01:25 +00:00

jlxq0 deleted branch feat/withings-mcp-server 2026-08-29 00:01:25 +00:00

jlxq0 referenced this pull request from a commit 2026-08-29 00:01:25 +00:00

Merge pull request 'Serialise every store mutation, not only refreshes' (#3) from feat/withings-mcp-server into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

jlxq0/withings-mcp!3

No description provided.