Lift a refusal by a switch rather than by taking the route away - #26
Conversation
`page.unroute` drops its handler while a request is in its hands, and that request is then neither continued nor refused: it never comes back, and nothing in the page times it out. The poll lost is not the cost. A send ends in a poll of its own, so a lift landing inside that poll leaves `post` awaiting forever — the widget the gesture was made on stays in `sending`, `reconcileState` skips it for the rest of the tab's life, and the page sits at `aria-busy` disagreeing with the log about a decision the log has taken. The assertion is what runs out, since nothing else does. That is the cross-tab settlement test's red on CI: the tab cut off from the log rejects a suggestion the log already holds an accept for, and the lift of its refusal lands inside the reject's own poll about one run in ten under load. Four of forty-eight loaded runs failed with the runtime's stage stuck at the minted event, the tab's poll counter one ahead of the requests the browser reported, and `data-lf-state` unwritten while the two live tabs had both folded to reject. So a refusal stays on the page for its whole life and answers every request it is handed; lifting changes which answer, not whether there is one. `CutOff` is that one handler, `held_stale` is it with the stamping poll let through, and the six sites that stopped a page's polls and started them again go through it. The seventh unroute is left with the reason it is safe: it holds and continues rather than refusing, its arrival is over, and the next one navigates away. Forty-eight loaded runs of the cross-tab test pass where the same load failed four, and twenty-four loaded runs of all seven converted tests pass. The complete suite reports 956 passed with `test_z_walks_back_through_gestures_ rather_than_toggling_one` red — the shape PR #24 is open for, red on main before this change and green alone here.
leaf-agent
left a comment
There was a problem hiding this comment.
Self-review. The diagnosis holds up and the conversion is faithful: every converted site keeps the same answer before the lift (lets_through=1 reproduces held_stale's old next(polls) == 0, lets_through=0 reproduces the bare refuse), and the two sites whose pattern widened from **/api/state to **/api/state* match the same request, since the runtime's only poll is fetch("/api/state") with no query. The arrive() exemption checks out too — the runtime schedules polls with setInterval, not chained off the previous one, so a stranded poll there costs that page nothing later.
Scoping the fix to /api/state and leaving the **/api/event unroutes alone is the right line, and the reason is worth having in the comment: /api/event is only ever requested by a gesture the test makes (or by reportPageError, which nothing awaits), so those unroutes never land on a request nobody asked for — where a timer's poll is exactly that. As written, the comment names the mechanism generally and then converts only the state sites, which reads as an oversight rather than a boundary.
One thing to drop below.
The pattern parameter had no caller and read as though the class were generic over any route; it only makes sense for the timer's poll, which is the request a lift can land in the middle of. Inlined, and the comment now states the boundary it had left implicit: an event is posted only by a gesture the test just made, so the api/event unroutes are where the mechanism stops applying rather than sites left unconverted.
|
Both actioned in b55a4d7. The
|
leaf-agent
left a comment
There was a problem hiding this comment.
Self-review of b55a4d7. The pattern inlining is right and every .hold(...) call site passes only page, so nothing breaks.
The new paragraph is the problem: it names **/api/event as the boundary, and that is not where the mechanism actually stops. Four page.unroute("**/api/state*") calls survive in the file, each dropping a live **/api/state* handler whose poll branch is refuse — the exact shape the header two paragraphs above forbids absolutely ("A refusal is lifted by a switch the handler reads, never by taking the route away"). Detail inline.
The PR description undercounts the same way: tests/test_render.py carries eleven unroute("**/api/state…") calls at the merge base, not seven. Six go through CutOff, one (arrive) gets the exemption comment, and four are unaccounted for. Worth correcting there too, since the description is what a reader checks the conversion against.
|
The four surviving |
The four `**/api/state*` unroutes still in the file arrived with main's merge — #21's own new tests — so the comment saying the `**/api/event` unroutes are the boundary was true of what this branch converted and false of the file it left. Convert them rather than write the exception down: the norm is absolute, and a survivor reads as one that was checked and found safe. Two are the converted shape exactly — refuse, lift, refuse again — which `CutOff` could not express because `restore` is one-way. `cut()` is the other half of that switch. Two lift a handler that answers as well as refuses, so the switch is a flag the handler reads at its top and continues on, written where the answers are.
leaf-agent
left a comment
There was a problem hiding this comment.
Self-review of the increment (16e68bf, b81fcfd). The conversion is complete and correct: **/api/state* now has exactly one surviving unroute, the exempt one in arrive(), and the four sites the last review named are converted. cut() reads right — with _live true the or short-circuits, so the count is never advanced by a live poll and a second cut restores the refusal without rewinding lets_through (I ran the switch table for lets_through 0 and 1 to confirm). The two hand-rolled lifted flags are sound too: rebinding a name the nested handler closes over is read at call time, so lifted = True reaches hold_older_state and malformed_poll_state, and both check the flag before the branch that would otherwise refuse or hold.
One thing, on what survives the merge. b81fcfd deleted the sentence that stated the norm — "A refusal is lifted by a switch the handler reads, never by taking the route away" — and it now exists nowhere in the repo. What remains is a three-line comment above one class in a 26k-line file, while tests/CLAUDE.md, which the root CLAUDE.md names as the owner of test rules, still says the opposite in the paragraph beginning "Every hold has a release path": "…continue or fulfill the route, wait for the handler to finish, remove the route, and only then close the page… Put release and unroute in cleanup that also runs when the assertion fails."
That paragraph is about a hold rather than a refusal, but it is the only place the mechanics file speaks about taking a route away, and **/api/state* still carries ~20 live route(…, refuse) registrations. The next test that needs to stop a page's polls and start them again reads that paragraph, reaches for unroute, and reopens the window this PR closed — with no gate anywhere to catch it, since the failure is a one-in-ten flake in an unrelated assertion. The fix this PR makes is durable in the file; the reason it was made is not.
A sentence in that paragraph would carry it — something like: a poll route is the exception, since the timer issues it whenever it comes round and a lift is free to land mid-request; switch its handler between refusal and continuation (CutOff) rather than removing it. Happy to push that as a commit if you want it, but the prose is yours — you just trimmed this material deliberately, so I'd rather ask than write over the call.
lint is green on 6fde164. test was still running when my poll cap expired, so I haven't verified it.
|
Merged before On the doc question, leaving it as your call and not pushing over it. The merged file keeps the mechanism in the comment above |
Problem
Run 32553975712 failed
test_a_decision_travels_between_tabs_and_the_log_has_the_last_word—expect(tab.locator("#sug-thistle lf-new")).to_be_hidden(), actualvisible— with the log holding the reject the assertion is about. The cause is the test's own instrument:page.unroutedrops its handler while a request is in its hands, and that request is then neither continued nor refused. It never comes back, and nothing in the page times it out.The lost poll is not the cost. A send ends in a poll of its own, so a lift landing inside that poll leaves
postawaiting forever — the widget the gesture was made on stays insending,reconcileStateskips it for the rest of the tab's life, and the page sits ataria-busydisagreeing with the log about a decision the log has taken. Here the cut-off tab rejects a suggestion the log already holds an accept for,third.unroute("**/api/state")lands inside the reject's own poll, and that tab never folds again.Solution
A refusal stays on the page for its whole life and answers every request it receives; lifting changes which answer, not whether there is one.
CutOffkeeps that handler installed and switches polling between refusal and continuation. On currentmain, eleven of the twelve polling-route lifts now use this pattern. The remainingarrive()unroute is safe because no send or later wait depends on that page, and the next call navigates away.Testing
The original failure reproduced four times in 48 loaded runs; the fix passed the same 48 runs. The current-main merge passed 13 affected browser tests. After the final conversion and simplification, the three directly changed tests passed and
pre-commitwas clean.The reading that named it
Three tabs,
thirdbeing the one cut off.pollscounts entries topoll(),asked/heardareTraffic's counts of what the browser reported:Seven polls, six requests: the third one's
fetchwas never issued to the network the browser reports on, and never settled.posttherefore never reached itsreturn minted,sendAction'sfinallynever ran, andsendingkeptsug-thistleout of every later reconciliation — the timer polls beside it kept working the whole time, which is whytold(third)returned and only the widget was wrong.What this does not change
The runtime's own behaviour under a poll that never returns is untouched: a stalled connection would pin a widget the same way, since
postawaits the read it ends with andsendingis released only behind it. That is reachable without any instrument, and #21 is already reworking that path, so it is left alone here rather than changed underneath it.Automated fix for failed run