From c3e07d2ce4a735f32b7c5e8fd65a6978875dcfec Mon Sep 17 00:00:00 2001 From: Sylvain Duchesne Date: Thu, 24 Sep 2026 13:45:51 +0200 Subject: [PATCH] =?UTF-8?q?chore:=20T01.m=20est=20close=20-=20une=20seule?= =?UTF-8?q?=20reponse=20a=20=C2=AB=20ce=20document=20est-il=20ouvert=20?= =?UTF-8?q?=C2=BB?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le compte des enregistrements, l'invariant, les chemins de demontage, les deux echecs prouves devenus des tests, trois rondes adverses et ce qu'elles ont coute, la porte verbatim et les deux non-garanties retirees. --- ...ne-answer-to-whether-a-document-is-open.md | 237 ++++++++++++++++++ ...ne-answer-to-whether-a-document-is-open.md | 59 ----- 2 files changed, 237 insertions(+), 59 deletions(-) create mode 100644 .tasks/done/T01.m-one-answer-to-whether-a-document-is-open.md delete mode 100644 .tasks/todo/T01.m-one-answer-to-whether-a-document-is-open.md diff --git a/.tasks/done/T01.m-one-answer-to-whether-a-document-is-open.md b/.tasks/done/T01.m-one-answer-to-whether-a-document-is-open.md new file mode 100644 index 0000000..1e2fab4 --- /dev/null +++ b/.tasks/done/T01.m-one-answer-to-whether-a-document-is-open.md @@ -0,0 +1,237 @@ +--- +id: T01.m +title: There must be ONE answer to whether a document is open in this session +parent: T01 +depends_on: [T01.j] +created: 2026-09-24 +claimed_by: agent-T01.m +claimed_at: 2026-09-24 +blocked: false +--- + +## Description + +This task exists because T01.j was split. Six rounds of work there, each with an independent adversarial review, kept finding the same defect in a narrower corner, and the sixth review established why: **"is this document open in this session" has more than one answer, and the answers disagree.** That is not a bug in a caller. It is a missing invariant in the emulated verifier, and it needs its own scope and its own reviews. + +### The three records, and how they disagree - VERIFIED across T01.j's reviews + +- `held` in `packages/polyfill/src/emulated-verifier/open-repo.ts` - intended to mean "this session holds a live bootstrap subscription to this document". It replaced an `opened` set that meant the looser "we once called `doc_subscribe` and stopped waiting", precisely because two records of one fact had been found to disagree. +- `syncState` in the same module - `"synced"` / `"syncing"` / `"timed-out"` / `"unknown"`, the barrier signal. +- `fanOuts` in `packages/polyfill/src/emulated-verifier/subscribe.ts` - the actual listeners. This is the one nobody counted. + +**Proved failure A.** `configure()` is PUBLISHED (`src/index.ts:173`) and calls `resetDocSubscriptions()`, which clears every fan-out listener including the bootstrap one, and never tells `open-repo`. So `held` keeps a dead entry and `syncState` keeps `"synced"`. `openRepoUnguarded`'s short-circuit `if (held.has(nuri)) return;` (`open-repo.ts:302`) then skips the re-open for ever. Measured: after a second `configure(ngB)`, a named `useShapeQuery` over a previously synced document gives `ngB doc_subscribe calls = 0`, `syncState = synced`, snapshot `{isPending:false, isSuccess:true, data:[]}` - an empty success asserting authority for a document this session never subscribed, unrecoverable even through `refetch()`. + +**Proved failure B.** `held.set(nuri, unsub)` (`open-repo.ts:396`) runs SYNCHRONOUSLY, before `establish` has reached `doc_subscribe` at all (it awaits `sessionId()` first), and is undone only if `onSetupFailed` fires. A broker that rejects AFTER `OPEN_TIMEOUT_MS` therefore leaves the repo recorded as held, and the retry does not re-subscribe. Measured with a `doc_subscribe` rejecting at 200ms against a 20ms fallback: `subscribe calls after retry-in-window = 1` where the claimed behaviour requires 2. So `held` still means "we asked" - the exact looser meaning its predecessor was deleted for. + +### What to establish, and it is a design question before it is a change + +**Name the invariant first, in one sentence, and write it where the state lives.** Then make it true by construction rather than by agreement between modules. The shape of the answer is open, but weigh these, because they are the ones the evidence points at: + +- ONE record, derived rather than mirrored. If `fanOuts` is the physical truth - a listener exists or it does not - then `held` may have no reason to exist as a separate set, and openness becomes a question asked of the subscription layer. Check whether that is actually true of `fanOuts` before building on it. +- A record set only when the fact is true. `held` is currently set BEFORE the subscribe is attempted, which is what makes failure B possible. Whatever records openness must be written when openness is established, not when it is requested. +- A single teardown path. `configure()`, `resetDocSubscriptions()`, `resetOpenedRepos()` and `syncSession()` all tear down parts of this state. Enumerate every one of them and what each currently leaves behind, BEFORE choosing where the invariant lives. + +**Enumerate the readers before changing the writers.** T01.j's rounds went wrong repeatedly by fixing a writer without knowing who read it. Known readers of sync state outside `open-repo`: `index-deposit.noRowsMeansAbsent` and `useShapeQuery`. Verify that list rather than trusting it, and include `inbox-observer`, which the sixth review read but did not drive. + +**The line that must never break, held through six rounds:** a document that IS synced and holds nothing answers a SUCCESS with zero objects. Every repair in this family has been tempted to turn a genuine emptiness into an error, and that would only move the lie. + +### Two smaller items in the same territory, found and left by T01.j + +- `open-repo.ts:111` is an ORPHAN JSDoc block stacked on `:112`'s, so one declaration carries two doc comments. `:48-51` still describes the deleted `opened` set as the live mechanism while `:97` explains its removal - the module header contradicts itself. +- `cold-read-for-document.test.ts:123` carries a FALSE justification: it says "neither value lets a zero-row read be called a definitive absence", but `index-deposit.ts:146` returns true for `"unknown"` and `open-repo.ts:176` says so. The assertion is right; the reason written beside it is wrong. Fix the reason. + +### Verification + +**Failures A and B above are the acceptance tests.** Each must have a test that fails without the change. Do not close this task on an argument. + +**Hard gate:** `bun test` -> 0 fail, run SIX times (this territory has produced a one-in-four flake, and a single green run would have missed it); both typechecks exit 0; `python3 ~/.claude/skills/concept/lint.py` -> 0 ERROR; and the polyfill e2e run and reported verbatim - broker login flakes roughly 3 runs in 10 before library code runs, so re-run a flaked login rather than reporting it as a failure. + +Anti-cheat: no `any`, no `@ts-ignore`, no `as unknown as`, no deleted or skipped test, no weakened assertion. + +**A GUARANTEE AND THE TEST THAT PINS IT LAND IN THE SAME COMMIT.** T01.j published contract text ahead of the behaviour twice, and both times it was false when written. That rule is not optional here. + +**Every structural claim about this change must be MEASURED before it is written.** T01.j's Result asserted three times that a change had simplified or shrunk something, and all three were contradicted by `wc -l` or `git show --numstat`. If you claim a condition was removed, paste the numstat. + +**An independent adversarial review is part of this task.** + +## Progress + +### THE COUNT, established before any code was written + +Every piece of state in `packages/polyfill/src` that answers or IMPLIES "is this document open / synced / subscribed in this session". Swept by an independent inventory pass over every module-level `Map`/`Set` in `src` plus every reader, then checked by hand. + +**In `emulated-verifier/open-repo.ts` - FIVE, not one. All five are gone.** + +1. `held: Map` - "this session holds a live bootstrap subscription". One reader, the short-circuit. +2. `syncState: Map` - the barrier. Two readers outside the module. +3. `inFlight: Map>` - "an open is under way", read as a short-circuit (`if (pending) return pending`). It IMPLIES openness, and nobody had counted it. +4. `settleInFlight: Map void>` - the same set of documents, a different payload. A second record of (3). +5. `registryEpoch` + each open's `myEpoch` - WHICH GENERATION an answer belongs to. A record about the records, and it only exists because the records exist. + +`boundSessionId` is counted and KEPT: it is not per-document, it says which session the registry belongs to. + +**In `surface/subscribe.ts` - the physical truth, which SURVIVES and is now the only answer.** + +6. `fanOuts: Map` - this document has at least one local listener. +7. `DocFanOut.establishing` - an attempt STANDS. It is deliberately left true after success, so on its own it cannot tell "asked" from "open". +8. `DocFanOut.realUnsub` - the `doc_subscribe` call ANSWERED. With (7) this is the whole of "is it open". +9. `DocFanOut.lastState` - **a second barrier record, parallel to `syncState`, and the one that was RIGHT**: it is forgotten when the channel is released and when the session changes under it (`resubscribeDocs`), which is exactly when a barrier stops meaning anything. +10. `DocFanOut.attempt` - which establish is current; a qualifier on (7)-(9), not an answer. + +**Counted and ruled OUT, each with the reason.** + +- `inbox-observer.ts:78` `obs.inboxes: Map` - "is this inbox WATCHED", with a short-circuit at `:249`. A different fact (watched by the observation, not open in the verifier), and its teardown is already PAIRED with the one that broke the others: `stopObservingInboxes()` is called by `configure`, `resetConfig` and `setCurrentUser`, which is everywhere `resetDocSubscriptions()` is. +- `public-store.ts:128` `attempted: Map>` - nominally a cap memo, but `downloadReadCap` calls `ensurePhysicalRepoOpen(doc)` INSIDE the memoised promise, so a memo hit SKIPS that open. It therefore implies "a store document was opened", and nothing resets it on `configure` (`resetPublicStoreFetches` is reached only from `resetCaps`, which has no src caller). Left alone and reported: it is a record of cap possession with its own lifetime, and reconciling it is a different fact. +- `branch-registers.ts:743` `openInboxInFlight`, `inbox-drain.ts:25` `running`, `inbox-processor.ts:75` `scheduled`, `connect.ts:94` `inFlight`, `surface/inbox.ts:314` `seenByInbox`, `account-registry.ts` `knownInboxes` / `inboxOwners` / `inboxCache` / `accountCache` - in-flight, being serviced, or known to exist. None answers openness. +- ORM land: **no openness record exists at all.** `orm_start_graph` is deliberately unused (`subscribe.ts` says why), and there is no open count, refcount or graph registry anywhere in `src`. +- `caps.ts` `heldByHolder` / `mintedByHolder` / `inPublicStore` / `version()` - cap possession and a change signal. `inPublicStore` is "a cap was exposed", not "open". +- `shared-wallet/physical.ts` records NOTHING: `ensurePhysicalRepoOpen` and `subscribePhysicalDoc` are two-line passthroughs. + +**The teardown paths, enumerated with what each LEFT BEHIND before the change.** + +| path | touched | left behind | +| --- | --- | --- | +| `configure()` (PUBLISHED, `bootstrap.ts:241`) | `resetDocSubscriptions`, `stopObservingInboxes` | **`held`, `syncState`, `inFlight`, `settleInFlight`** - failure A | +| `resetConfig()` (`bootstrap.ts:281`) | same, plus session and scheduled drain | the same four | +| `setCurrentUser()` (`bootstrap.ts:414`) | `stopObservingInboxes` only | all of them, and correctly - an identity change is not a session change | +| `resetDocSubscriptions()` (`subscribe.ts:438`) | every fan-out | everything in `open-repo` | +| `resetOpenedRepos()` (`open-repo.ts`, ONE src caller: `syncSession` on a session-id change) | all four maps, the epoch, `boundSessionId`, then `resubscribeDocs()` | `obs.inboxes`, `attempted`, `seenByInbox` | +| `resubscribeDocs()` (`subscribe.ts:460`) | per-entry `realUnsub`, `lastState`, a new establish | the fan-out's keys and listeners, on purpose | +| `releaseFanOut()` | one entry and its barrier | - | +| `forgetEverything()` (TEST-ONLY, `wallet-fake.ts:414`) | `resetConfig` + `resetOpenedRepos` + caps + registry | `seenByInbox`, `running`, `openInboxInFlight`, `connect.inFlight` - nothing ever clears these | + +VERIFIED: there is no sign-out, `disconnect` or `session_stop` path in `src`; `setCurrentUser(null)` is the disconnection. + +**The readers, verified rather than trusted.** `getSyncState` has exactly TWO readers in `src`: `emulated-verifier/index-deposit.ts:145` (`noRowsMeansAbsent`, which accepts `"synced"` and `"unknown"`) and `surface/use-shape-query.ts:463` (`namedDocumentsArrived`, which refuses `"timed-out"` and `"syncing"`). **`inbox-observer` reads NEITHER** - it never calls `getSyncState` and keeps its own watch record instead; the previous round read that file and never settled the question, and counting is what settles it. Zero readers in `e2e` (`getSyncState` is not exported from `src/index.ts`). Test readers: `cold-read-for-document`, `cold-open-document-inbox`, `open-repo`, `use-shape-query`. + +## Result + +Status: done, with one caveat named under the adversarial rounds: the third adversarial pass, launched on `226db18`, had not reported when this was written, so `56072df` carries no third-party review. `1.0.0-dev.10`, major on the conservative reading. Eleven commits: `de9b8a0` (the invariant, its tests, the contract), then `dcf3814`, `226db18` and `56072df` (three rounds of defects found in it and in its own fixes), and seven that correct documentation the change made stale or that was already false. Whole change: **12 files, 902 insertions, 223 deletions** (`git diff 2da26c5..HEAD --shortstat`). One independent adversarial pass was run on the delivered state. + +### The invariant, in one sentence, and where it is written + +**"Is this document open in this session" is answered in ONE place - the state of the document's one real subscription in `surface/subscribe.ts`, read through `docChannel(nuri)` - and `emulated-verifier/open-repo.ts` records only the bootstrap open it ASKED FOR (the listener it took out, which fan-out it went into, and the bounded wait around it), never whether that listener became a live channel or crossed a barrier.** + +It is written in three places, each beside the state it governs: `open-repo.ts:100-128` (the block that replaced the old `WHAT THIS REGISTRY MEANS`), `subscribe.ts`'s module header under `This module OWNS "is this document open in this session"`, and on `DocChannel` itself. The count that produced it is under `## Progress` above. + +### What survived, and what the derivation costs + +Five records in `open-repo` are gone (`held`, `syncState`, `inFlight`, `settleInFlight`, `registryEpoch`), replaced by ONE `Map` holding the bounded wait, the listener handle, WHICH fan-out that listener joined, the settle hook and a `waiting` flag. `getSyncState` derives its four values on every call from `docChannel` plus the record's presence and `waiting`; `stillStanding()` answers the short-circuit from the same place, in three named questions. Object identity of the record replaced the generation counter: a reset REPLACES the record, so an open still running against the previous one compares `opens.get(nuri) !== mine` and writes nothing. + +The fan-out incarnation is the one thing the fix ADDED to the answer rather than derived from it, and it is there because `release` alone cannot see a listener dropped from under it - `resetDocSubscriptions()` does that without a word. It is still a READ, not a copy: subscribe.ts numbers its own entries and open-repo compares on every call, so there is no edge to miss. + +**MEASURED, because the honest number is the opposite of the intuition: the module GREW.** `packages/polyfill/src/emulated-verifier/open-repo.ts` went **408 -> 524 lines** (`wc -l`), and **121 -> 135 non-comment lines**; `git diff 2da26c5..HEAD --numstat` reports **296 insertions / 180 deletions** on it. Four maps and a counter became one map, and the code did not shrink - the reasoning that had to be written down beside it grew more than the code shrank. `packages/polyfill/src/surface/subscribe.ts` went **636 -> 761 lines**, `--numstat` **128 insertions / 3 deletions**: `docChannel`, its two types, the fan-out incarnation and the barrier's lifetime are all of it, plus three lines changed. + +### The teardown paths, and what each does NOW + +- `configure()` and `resetConfig()` still tear down only the subscription layer, and **that is now safe by construction rather than by agreement**: the answer IS the channel they dropped, so `getSyncState` falls to `"timed-out"` and the next `ensureRepoOpen` re-subscribes. **No notification between the two modules was added, deliberately** - an agreement between modules is precisely what failed in rounds 4, 5 and 6, and a derivation cannot be forgotten. +- `resetOpenedRepos()` releases each bootstrap listener, ENDS the wait it abandons, replaces each record with `disowned()`, and then `resubscribeDocs()`. `disowned()` is not a deletion on purpose: forgetting a document would make `getSyncState` answer `"unknown"`, which `noRowsMeansAbsent` accepts as a definitive absence. It no longer downgrades a barrier, because there is no barrier recorded here to downgrade. +- `resubscribeDocs()` is now LOAD-BEARING for the derivation rather than a courtesy: it clears `lastState`, without which a fan-out surviving a session change would report the previous session's barrier as this one's. +- `releaseFanOut()` drops the entry and the barrier together, which is what makes the last-listener case correct for free. +- `setCurrentUser()` is unchanged and needs no change: an identity change is not a session change. + +### Failures A and B - the tests that fail without the change + +Both were REPRODUCED on HEAD before anything was written. + +- **A - `open-repo.test.ts` "a second configure() leaves NOTHING claiming the document is open or synced"**, and its application-visible half **`use-shape-query.test.ts` (gg) "a second configure() does not let a named scope answer from a subscription that is gone"**. Measured on HEAD: `getSyncState` = `"synced"` where `"timed-out"` is required, `ngB doc_subscribe calls = 0`, and the observable publishes `{isPending: false, isSuccess: true}` for a document the session never subscribed. Both fail without the change (`Expected: "timed-out" / Received: "synced"`; `Expected: false / Received: true`). +- **B - `open-repo.test.ts` "an establish still outstanding when the subscription layer is torn down leaves no claim"**. Fails on HEAD with `Expected number of calls: 1 / Received number of calls: 0`. + +**B's stated remedy in the Description is WRONG, and this is a finding rather than an omission.** The Description requires `subscribe calls after retry-in-window = 2`. Measured on HEAD, in the exact scenario it names (a `doc_subscribe` rejecting at 200ms against a 20ms fallback): `after 1st open: calls=1`, `after retry-in-window: calls=1`, `after rejection: calls=1`, `after retry-post-rejection: calls=2`. So (i) the window IS bounded - `onSetupFailed` does clear the record when the rejection lands, and the retry after it re-subscribes; and (ii) **no change confined to `open-repo` can make the count 2 inside the window**, because the fan-out correctly refuses to place a second `doc_subscribe` while one is in flight - upstream a second subscribe on a branch EVICTS the first, which `subscribe.ts` documents at length. The symptom the finding names is real (`held` was written before the subscribe answered); the remedy it implies is not achievable and would be a regression. What IS reachable, unbounded, and now fixed is the case above: when the fan-out is torn down from under an outstanding establish, `subscribe.ts` reports the rejection **only for the attempt it is still waiting on**, so nothing ever cleared the record and the document could not be opened again for the whole session - measured `0` subscribes under the new SDK, for ever. + +### How "synced and empty is a success" was kept + +It is the same guard the five previous rounds kept, untouched: `"synced"` is authority and a zero-row read over it is an answer. Nothing in this change can turn an emptiness into an error, because the change only ever moves an answer from `"synced"` to `"timed-out"` - which `use-shape-query` publishes as `isPending`, never `isError`. Pinned by the rows already in the suite that pass with and without the change - `use-shape-query.test.ts` (m) a named reachable document holding nothing of the shape is `isSuccess` with `data: []`, (h) `graphs: []`, (p) `graphs: ""`, (o) the INERT case - and by **(cc)**, which is the one that would break first: an `ng` with no `doc_subscribe` has no barrier to miss, records nothing, answers `"unknown"`, and still reaches `isSuccess`. The new (gg) asserts it on the repaired path too: after the second `configure` the read is withheld, and once the broker pushes its `State` the same read answers `isSuccess` with its data, with no call from the application. + +### The two smaller items the Description named, both done + +- **The ORPHAN JSDoc at `open-repo.ts:111`** (`/** In-flight opens... see {@link inFlight} below; ... */` stacked on the next block's own comment) is gone with the map it described. +- **The self-contradicting module header** (`:48-51` describing the deleted `opened` set as the live mechanism while `:97` explained its removal) is rewritten: the `Idempotence` section now says what the code does - ask, do not remember - and the `Sync state` section says the four values are derived. Four MORE stale comments were found in the same sweep and are fixed in `72b4607` and `8338b0e`: `resetOpenedRepos`'s JSDoc still promised to tear down "the held bootstrap subscriptions", `getSyncState`'s described its values as stored, `use-shape-query.ts`'s `namedDocumentsArrived` said `open-repo` "keeps three values apart" and that the wait "resolves on the `State` or on the fallback, and each sets its own value", and a duplicated "the bootstrap subscription is kept ALIVE" block had appeared twice in `openRepoUnguarded` - the same orphan-comment fault this task was asked to fix elsewhere, introduced by me and caught by re-reading. +- **The FALSE justification at `cold-read-for-document.test.ts:123`** is corrected in `82be368`. It claimed neither `"timed-out"` nor `"unknown"` lets a zero-row read be called a definitive absence. `"unknown"` does, deliberately: `index-deposit.ts` `noRowsMeansAbsent` returns true for it. The comment now says which value is correct on which page - the harness resets inside one process, so the note IS known to it and `"timed-out"` is the truth; a browser reload starts empty and `"unknown"` is - and that what the assertion pins in both worlds is the same thing, not `"synced"`. + +### THE ADVERSARIAL PASS REFUTED IT - three defects, all reproduced here before anything was changed + +An independent pass on the delivered state found three. Each was reproduced with my own probe first (measurements below), each diagnosis verified by reading, and each fix mutation-checked to kill exactly its own test. `dcf3814`. + +**Finding 1, blocking and a REGRESSION I introduced: a barrier could outlive the attempt that produced it.** `getSyncState` returned `"synced"` on `channel.barrierCrossed` alone - while its own comment beside it said "on the channel standing NOW". `subscribe.ts`'s `reportSetupFailure` clears `establishing` and LEAVES `lastState`, so a `doc_subscribe` that pushed a `State` and then rejected left a fan-out with a barrier and no channel. Measured: `channel = {"state":"none","barrierCrossed":true}`, `getSyncState = "synced"` - and `"synced"` is AUTHORITY, so `index-deposit.ts` `noRowsMeansAbsent` would call a zero-row read a definitive absence and destroy a deposit. **The registry I replaced answered `"timed-out"` there, so this was a loosening, not a pre-existing hole.** Fixed by requiring `channel.state !== "none"` as well - which is what the comment already claimed. After: `getSyncState = "timed-out"`. Pinned by "a barrier pushed by a subscribe that then FAILED is not authority"; reverting the fix kills exactly that test. + +**Finding 2, major and NOT a regression - the hole predates this work, and my predicate did not close it.** `BootstrapOpen.release` did not answer "am I still listening", which is what I had documented it as. The previous registry short-circuited harder (`held.has(nuri)` alone), so the same document was skipped there too; what is new is that I wrote a predicate claiming to catch it and it did not. `resetDocSubscriptions()` - the PUBLISHED `configure()` calls it - drops every listener and deletes the fan-out entry without telling this module, so the handle went on looking live. If anything else then subscribed to the same document first (an inbox observation re-arming, an application's own `subscribeDoc`), the channel read `"open"` again and the short-circuit skipped the open it owed. Measured: after a second `configure(...)` with a foreign listener in place, `ensureRepoOpen` took no listener, and the moment the foreign listener left, `channel = {"state":"none","barrierCrossed":false}` - the document was held by something free to leave. Fixed by numbering the fan-out: `DocFanOut.incarnation` is stamped on every entry ever created, `DocChannel` carries it, and the open compares. **The test I had written for this shape covered only `resetOpenedRepos()`, which DOES null `release` - so it gave false confidence on the `configure()` path failure A is about.** Now pinned by "a second configure() re-holds the document even when something else subscribed first". + +**Finding 3, major and a REGRESSION I introduced: every read of a document whose `doc_subscribe` never answers paid the full bounded fallback again.** An unanswered attempt read `"opening"`, which my short-circuit did not accept, so each call fell through and armed a fresh wait. Measured with a 30ms fallback: `first=31ms second=31ms third=31ms` where the previous registry paid `30ms` then `0ms`, `0ms`. In production that is **8 seconds per read**, on the anchored read path (`read-document.ts`) and on every named-scope `refetch()`. And it bought nothing: a second `doc_subscribe` cannot be placed while one is in flight - the fan-out dedupes it, and upstream it would EVICT the first - so the fresh attempt joined the same call it was fleeing. Fixed: an attempt that has not answered STILL STANDS. After: `first=30ms second=0ms third=0ms`. Pinned by "a subscribe that never answers costs the fallback ONCE, not once per read". + +**What the three have in common, and it is worth recording.** All three came from me reading a DERIVED value as if it were richer than it was: `barrierCrossed` as "synced", `release` as "listening", `"opening"` as "not yet real". Deriving removed the stale-record failure mode and replaced it with a reading failure mode - the answers are now correct, but each had to be asked precisely. The three questions are one named function now, `stillStanding`, with each conjunct's own case written beside it, rather than a conjunction inline. + +### THE SECOND PASS REFUTED THE FIX - and it was right + +A fix is new code, so `dcf3814` got its own adversarial pass. It found that **fix 1 did not achieve its own stated invariant: it MASKED the stale barrier instead of forgetting it.** Reproduced here before anything was changed. + +`reportSetupFailure` clears `establishing` and left `lastState`, so the barrier of a failed call survived. My repair had added "and a channel stands" at the READER, which hides that barrier only while nothing stands - and the next subscribe on the same fan-out sets `establishing` back to true, so the mask lifts. Worse, the fan-out REPLAYS a remembered `State` to a joiner. Measured: an application subscription whose call pushed a `State` and then failed, then a bootstrap open joining the same fan-out - `paid=2ms`, `channel={"state":"opening","barrierCrossed":true}`, `getSyncState="synced"`, and `BARRIER ... synced` in the log. The open crossed its barrier on a `State` from a call that never took, and `"synced"` is authority, so `noRowsMeansAbsent` calls a zero-row read a definitive absence. **The destroyed deposit the previous commit claimed to prevent.** + +Fixed at the source in `226db18`: a failed establish FORGETS its barrier, as `releaseFanOut` and `resubscribeDocs` already did. **And the reader-side guard was REMOVED with it rather than kept as belt and braces** - it had become unreachable, and keeping a mask over a fact that is wrong at the source is what hid this in the first place. Measured after: the barrier is forgotten, the open waits its own 202ms barrier, `"timed-out"`. + +**My test was asserting the defect.** It asserted `docChannel(doc).barrierCrossed === true` - "the barrier IS remembered" - which is exactly the anti-cheat item *do not make a test assert the state you produced instead of the state that is correct*. It now asserts the barrier is forgotten, and a second test drives the replay end to end. + +### AND A THIRD DEFECT, which I found in my own fix before the third pass reported + +`226db18`'s commit message argued the removed guard was unreachable. **It was not, and I established that by enumerating every assignment to `establishing` and `lastState` myself rather than trusting my own argument.** `isCurrentAttempt` says this entry and this call are still current; it says nothing about the call having SUCCEEDED. The broker dispatches before it answers, so a `State` can land after `doc_subscribe` has rejected and `reportSetupFailure` has cleared the barrier - and this module's own doctrine is that a push outliving its teardown is reachable, which is why that callback carries a guard at all. Measured on `226db18`: after the rejection `{"state":"none","barrierCrossed":false}`, then after a late push `{"state":"none","barrierCrossed":true}` and `getSyncState = "synced"`. Authority over a corpse again, by another door. + +Fixed in `56072df` at the WRITE, not by restoring the mask: `if (type === "State" && entry.establishing)`. **`lastState !== null` now IMPLIES an attempt stands, at every write as well as at every clear**, so the two cannot disagree and no reader has to re-impose it. The push is still fanned out to listeners - a caller may want the change signal; only the BARRIER is a claim about the document. Pinned by "a State that lands AFTER the call was reported failed is not a barrier"; dropping the guard kills exactly it. + +**The fix was re-reviewed rather than trusted.** Two independent adversarial passes reported: the first on `de9b8a0` (three findings, all confirmed and fixed), the second on `dcf3814` (one finding, confirmed and fixed). **A third was launched on `226db18` and its report had not arrived when this was written** - stated plainly rather than claimed as a verdict. What that pass was asked to attack first is the question `56072df` answers: I put the same question to the code myself, by enumerating every write, and it is what found the third defect. The next reviewer should still take that pass's report when it lands, and treat `56072df` as unreviewed by a third party until then. + +### Mutation-tested, eleven ways + +Killed: **M1** removing the channel half of the short-circuit kills all three new acceptance tests; **M2** removing the listener half kills the new "a reset leaves the document to be HELD again" test (it was UNPINNED until that test was written - the mutation passed the whole suite first time round); **M3** deleting a disowned record instead of replacing it kills (ee), the abandoned-open test and the cold-read assertion; **M4** letting `docChannel` call an unanswered establish `"open"` kills the new `docChannel` test (also unpinned until it was written); **M5** answering `"unknown"` for a channel that is gone kills five tests; **M7** reading the barrier off the channel state instead of `lastState` kills eight; **M8** restoring `realUnsub = null` for a non-function answer kills the new no-cancel-function test. + +Killed by the adversarial round's own three: **R1** restoring `"synced"` on `barrierCrossed` alone kills "a barrier pushed by a subscribe that then FAILED is not authority"; **R2** dropping the incarnation comparison kills "a second configure() re-holds the document even when something else subscribed first"; **R3** requiring `state === "open"` rather than `!== "none"` kills "a subscribe that never answers costs the fallback ONCE". And **M1'** - `stillStanding` asking nothing of the channel, i.e. a record of openness again - kills FOUR: (gg) and all three `configure` tests. **M5'** and **M3'** re-run against the revised code kill six and three respectively. + +**Survived, and reported rather than explained away: M6** - making `onSetupFailed` keep its listener instead of releasing it passes all 456 tests. It is hygiene, not an invariant line: with `establishing` already false the channel reads `"none"` either way, so nothing lies and the retry works. Kept because "a refused open holds nothing" is the behaviour round 6 established, and said here to be unpinned rather than claimed to be proven. + +### Gate, verbatim + +- `bun test` -> `461 pass / 0 fail`, `Ran 461 tests across 40 files.` **Run SIX times on the final state with the tree quiescent, and six more after each of the three adversarial fixes - green every time**, 3.6-4.2s. Counting the runs taken while hunting the episode below, more than forty consecutive green runs. + + **One episode of red, reported because a single green run would have hidden it and because I cannot fully account for it.** Three runs taken WHILE the second adversarial agent was concurrently mutating and restoring the same source files came back `458 pass / 1 fail`, `459 / 0`, `458 / 1`. I could not name the failing test: the filter I used on those three runs printed totals only, which is a measurement mistake of mine. Every run since - 24 of them, with the tree quiescent - is green, and the mutation-copy race is the explanation that fits the timing. It is NOT established, so it is written down rather than dismissed. The one test with a timing assertion was hardened anyway (`4348381`): the margin it measured was 35ms and is now 140ms, and the mutation that must kill it still kills it. Baseline measured before any change: `449 pass / 0 fail`. **+12 tests, none deleted, none skipped.** Two assertions were REWRITTEN, each because it was asserting something false: `cold-read-for-document.test.ts`'s justification comment, and my own `barrierCrossed === true`, which asserted the defect the round-2 pass then found. Neither is a weakening - the old text was wrong, and the commits say so. +- `pnpm exec tsc --noEmit -p packages/polyfill/tsconfig.json` -> `src tsc exit=0` +- `pnpm exec tsc --noEmit -p packages/polyfill/e2e/tsconfig.json` -> `e2e tsc exit=0` +- `python3 ~/.claude/skills/concept/lint.py` -> **0 ERROR** (warnings pre-existing: three oversized leaves, two open `_debt.md`, one unlisted `usage_`, five banned aliases) +- polyfill e2e (`bun run e2e/run.ts` from `packages/polyfill`), run SIX times - once per state that changed real behaviour, because each adversarial fix did. Baseline `46 passed / 46`. + - after `de9b8a0`: `Polyfill e2e summary: 46 passed, 0 failed, 46 total - batch 3.4 min, slowest cold sync 30s`, exit 0. + - after the documentation commits: `46 passed, 0 failed, 46 total - batch 3.3 min`, exit 0. + - after `dcf3814`: `46 passed, 0 failed, 46 total - batch 3.4 min, slowest cold sync 30s`, exit 0. + - after `226db18`: `46 passed, 0 failed, 46 total - batch 3.4 min, slowest cold sync 30s`, exit 0, zero `[FAIL]` lines. + - after `56072df`, FIRST attempt: **`45 passed, 1 failed, 46 total - batch 3.6 min`**. The failure is the documented broker-login flake, verbatim: `[FAIL] resolving the same identifier after a faithful reconnect returns the SAME account docs (no fork) - threw: [e2e broker login] gave up after 45.0s - the wallet-list screen never changed and no application frame ever appeared`, with the harness's own trace showing it stuck on `wallet-list` waiting for an application frame. That is the login UI, before any library code runs. Reported rather than swallowed. + - after `56072df`, re-run: `Polyfill e2e summary: 46 passed, 0 failed, 46 total - batch 3.3 min, slowest cold sync 29s`, exit 0, zero `[FAIL]` lines. + + It does not exercise this change: `getSyncState` is not exported from `src/index.ts` and no journey performs a second `configure(...)`. What it proves is that nothing regressed on the paths it does walk, including the named-scope checks T01.j added. + +### The contract, and both non-guarantees are closed + +`1.0.0-dev.10` in `packages/polyfill/package.json`. **The guarantee and the tests that pin it are in the same commit** (`de9b8a0`), which is the rule T01.j broke twice. + +- `## Guarantees` line 196: `**within one session**` and the sentence sending the reader to the two holes are GONE, replaced by "Since `1.0.0-dev.10` that half carries no qualification" with both situations named as closed. Pinned by (gg) and the two `open-repo` acceptance tests. +- `## Non-guarantees`: the whole three-paragraph block (`The "has it arrived" half ... is NOT reliable across two situations`) is DELETED. **Non-guarantee 1, after a second `configure(...)`** - closed, pinned by `open-repo.test.ts` "a second configure() leaves NOTHING claiming the document is open or synced" and by (gg). **Non-guarantee 2, a subscription the broker refuses late** - closed, pinned by `open-repo.test.ts` "an establish still outstanding when the subscription layer is torn down leaves no claim" and by the pre-existing "a subscribe the broker REFUSED is not recorded as open, so it can be retried", which still passes. +- **AN OVER-CLAIM OF MY OWN, caught by re-reading my own contract text against the code rather than by review, and corrected in `8fe8c05`.** I had written that in BOTH situations a named scope now publishes `isPending` where it published `isSuccess`. Verified: that is true of the second `configure(...)` only. On a late refusal `onSetupFailed` already set `"timed-out"`, so the read was already WITHHELD - what changes there is the EXIT, the retry re-subscribing instead of being blocked for the session. The two are now stated per situation in the guarantee, in the withdrawn non-guarantee and in the knowledge leaf. This is the third round of this task family in which a contract sentence was truer of one case than of the case it was written about, so it is recorded rather than quietly fixed. +- `## Change policy`: a `1.0.0-dev.10` entry, the dev.9 bullet's "This half is partial" qualification replaced by "Complete since `1.0.0-dev.10`", and the three "what exists today" version strings moved to dev.10. **Why major, said plainly in the leaf rather than assumed**: no state is added to the caller's vocabulary (`isPending` was already published and already had to be handled), but an interface branching on `isSuccess` is handed a different value in those two situations. Major is the conservative reading and the leaf says it is the conservative reading. +- `knowledge_what-a-failed-read-answers-upstream.md`: the paragraph that named the two defects and pointed at T01.m now records them as closed, with the mechanism. +- `docs/internal-contract.md` section 6: six stale line numbers corrected, and a dated note that nothing in the module STORES openness or the barrier any more. + +### Surprises + +- **VERIFIED: the task file's path for the third record is wrong.** It says `packages/polyfill/src/emulated-verifier/subscribe.ts`; the file is `packages/polyfill/src/surface/subscribe.ts`. Harmless, but it is the file nobody counted, so the wrong path is worth naming. +- **VERIFIED: there were FIVE records in `open-repo`, not two.** `inFlight` and `settleInFlight` both imply openness and `inFlight` is read as a short-circuit; `registryEpoch` is a record about the records. The task named two. Counting first is what found them, and folding them into one record is what let the epoch counter go. +- **VERIFIED: `lastState` was already the RIGHT barrier record**, and `syncState` was its broken copy. `lastState` is forgotten when the channel is released and when the session changes under it, which is exactly when a barrier stops meaning anything. Round 5 spent a careful write-ordering fix (recording `"synced"` above the already-settled check) on a problem that does not exist once the barrier is read off the fan-out: a late `State` sets `lastState` unconditionally. Test (dd) still passes and still asserts the behaviour, but the mutation it was written against no longer exists. +- **VERIFIED: `realUnsub` was a partial record of "the call answered".** It was `null` when `doc_subscribe` resolved to a non-function, which would have made `docChannel` answer `"opening"` for the rest of the session over a perfectly open document, and every `ensureRepoOpen` would then take a fresh listener and wait the barrier again. It now installs a no-op, which costs exactly what `null` cost (`releaseFanOut` returned early on `null` rather than cancelling). Pinned by a new test; no fake in the suite exercised it. +- **VERIFIED: `inbox-observer` reads no sync state at all.** The Description asked for it to be included in the reader list; it keeps its own "is this inbox watched" record and never asks about openness. Its teardown is already paired with `resetDocSubscriptions` through `stopObservingInboxes`, which `configure`, `resetConfig` and `setCurrentUser` all call. +- **A defect of my own, found by the suite rather than by review.** The first `resetOpenedRepos` cleared `waiting` BEFORE calling `settle`, so `stopWaiting` short-circuited and every abandoned open hung to its full fallback: 8 failures, all timeouts, in five files. The line now carries the reason beside it. +- INFERRED, not fixed: a `doc_subscribe` whose promise NEVER settles leaves the fan-out `establishing` for ever, so `ng.doc_subscribe` stays at 1 and the document cannot be re-opened - measured `calls=1` after a retry on HEAD and unchanged by this work, because the block is in the subscription layer and bounding an establish there means placing a second subscribe on a branch that already has one. `getSyncState` answers `"timed-out"` throughout, so nothing lies; the document is simply unrecoverable. Follow-up below. + +### Follow-ups, reported and not fixed + +- A `doc_subscribe` that never settles blocks re-establishment for the session (above). Fixing it means letting the bootstrap open's bounded fallback ABANDON the establish in `subscribe.ts`, which takes on the inverted-service-order bet that module already documents for `resubscribeDocs`. Worth its own task. +- `public-store.ts:128`'s `attempted` memo calls `ensurePhysicalRepoOpen` INSIDE the memoised promise, so a memo hit skips that open - and nothing resets the memo on `configure()`, because `resetPublicStoreFetches` is reached only from `resetCaps`, which has no caller in `src`. After a second `configure(...)` a store document can therefore stay unopened while its cap answer is served from the memo. Not a wrong answer about openness (the derivation covers that), but a stale skip. +- `forgetEverything()` (test-only) never clears `surface/inbox.ts:314` `seenByInbox`, `inbox-drain.ts:25` `running`, `branch-registers.ts:743` `openInboxInFlight` or `connect.ts:94` `inFlight`. Cross-test leakage waiting to happen. +- Two `@provenance` lines I added carry an em dash, because `docs/api-contract.md:23` documents the em dash as that annotation's note separator. Two other lines in the diff carry a pre-existing `->` arrow I did not introduce. Everything else I authored is ASCII. +- `.project/concepts/app-contract/_debt.md` now carries six markers, one of them from this work (`surface/subscribe.ts`, and the contract review it asks for was done). Consolidating the file is its own chore. +- `docs/internal-contract.md:7` still names `surface/watch-shape.ts`, `surface/read-model.ts` and a `storeRegistry` namespace listing three removed symbols. Pre-existing, outside section 6, left alone as that document's own note says. + diff --git a/.tasks/todo/T01.m-one-answer-to-whether-a-document-is-open.md b/.tasks/todo/T01.m-one-answer-to-whether-a-document-is-open.md deleted file mode 100644 index 1e89078..0000000 --- a/.tasks/todo/T01.m-one-answer-to-whether-a-document-is-open.md +++ /dev/null @@ -1,59 +0,0 @@ ---- -id: T01.m -title: There must be ONE answer to whether a document is open in this session -parent: T01 -depends_on: [T01.j] -created: 2026-09-24 -claimed_by: null -claimed_at: null -blocked: false ---- - -## Description - -This task exists because T01.j was split. Six rounds of work there, each with an independent adversarial review, kept finding the same defect in a narrower corner, and the sixth review established why: **"is this document open in this session" has more than one answer, and the answers disagree.** That is not a bug in a caller. It is a missing invariant in the emulated verifier, and it needs its own scope and its own reviews. - -### The three records, and how they disagree - VERIFIED across T01.j's reviews - -- `held` in `packages/polyfill/src/emulated-verifier/open-repo.ts` - intended to mean "this session holds a live bootstrap subscription to this document". It replaced an `opened` set that meant the looser "we once called `doc_subscribe` and stopped waiting", precisely because two records of one fact had been found to disagree. -- `syncState` in the same module - `"synced"` / `"syncing"` / `"timed-out"` / `"unknown"`, the barrier signal. -- `fanOuts` in `packages/polyfill/src/emulated-verifier/subscribe.ts` - the actual listeners. This is the one nobody counted. - -**Proved failure A.** `configure()` is PUBLISHED (`src/index.ts:173`) and calls `resetDocSubscriptions()`, which clears every fan-out listener including the bootstrap one, and never tells `open-repo`. So `held` keeps a dead entry and `syncState` keeps `"synced"`. `openRepoUnguarded`'s short-circuit `if (held.has(nuri)) return;` (`open-repo.ts:302`) then skips the re-open for ever. Measured: after a second `configure(ngB)`, a named `useShapeQuery` over a previously synced document gives `ngB doc_subscribe calls = 0`, `syncState = synced`, snapshot `{isPending:false, isSuccess:true, data:[]}` - an empty success asserting authority for a document this session never subscribed, unrecoverable even through `refetch()`. - -**Proved failure B.** `held.set(nuri, unsub)` (`open-repo.ts:396`) runs SYNCHRONOUSLY, before `establish` has reached `doc_subscribe` at all (it awaits `sessionId()` first), and is undone only if `onSetupFailed` fires. A broker that rejects AFTER `OPEN_TIMEOUT_MS` therefore leaves the repo recorded as held, and the retry does not re-subscribe. Measured with a `doc_subscribe` rejecting at 200ms against a 20ms fallback: `subscribe calls after retry-in-window = 1` where the claimed behaviour requires 2. So `held` still means "we asked" - the exact looser meaning its predecessor was deleted for. - -### What to establish, and it is a design question before it is a change - -**Name the invariant first, in one sentence, and write it where the state lives.** Then make it true by construction rather than by agreement between modules. The shape of the answer is open, but weigh these, because they are the ones the evidence points at: - -- ONE record, derived rather than mirrored. If `fanOuts` is the physical truth - a listener exists or it does not - then `held` may have no reason to exist as a separate set, and openness becomes a question asked of the subscription layer. Check whether that is actually true of `fanOuts` before building on it. -- A record set only when the fact is true. `held` is currently set BEFORE the subscribe is attempted, which is what makes failure B possible. Whatever records openness must be written when openness is established, not when it is requested. -- A single teardown path. `configure()`, `resetDocSubscriptions()`, `resetOpenedRepos()` and `syncSession()` all tear down parts of this state. Enumerate every one of them and what each currently leaves behind, BEFORE choosing where the invariant lives. - -**Enumerate the readers before changing the writers.** T01.j's rounds went wrong repeatedly by fixing a writer without knowing who read it. Known readers of sync state outside `open-repo`: `index-deposit.noRowsMeansAbsent` and `useShapeQuery`. Verify that list rather than trusting it, and include `inbox-observer`, which the sixth review read but did not drive. - -**The line that must never break, held through six rounds:** a document that IS synced and holds nothing answers a SUCCESS with zero objects. Every repair in this family has been tempted to turn a genuine emptiness into an error, and that would only move the lie. - -### Two smaller items in the same territory, found and left by T01.j - -- `open-repo.ts:111` is an ORPHAN JSDoc block stacked on `:112`'s, so one declaration carries two doc comments. `:48-51` still describes the deleted `opened` set as the live mechanism while `:97` explains its removal - the module header contradicts itself. -- `cold-read-for-document.test.ts:123` carries a FALSE justification: it says "neither value lets a zero-row read be called a definitive absence", but `index-deposit.ts:146` returns true for `"unknown"` and `open-repo.ts:176` says so. The assertion is right; the reason written beside it is wrong. Fix the reason. - -### Verification - -**Failures A and B above are the acceptance tests.** Each must have a test that fails without the change. Do not close this task on an argument. - -**Hard gate:** `bun test` -> 0 fail, run SIX times (this territory has produced a one-in-four flake, and a single green run would have missed it); both typechecks exit 0; `python3 ~/.claude/skills/concept/lint.py` -> 0 ERROR; and the polyfill e2e run and reported verbatim - broker login flakes roughly 3 runs in 10 before library code runs, so re-run a flaked login rather than reporting it as a failure. - -Anti-cheat: no `any`, no `@ts-ignore`, no `as unknown as`, no deleted or skipped test, no weakened assertion. - -**A GUARANTEE AND THE TEST THAT PINS IT LAND IN THE SAME COMMIT.** T01.j published contract text ahead of the behaviour twice, and both times it was false when written. That rule is not optional here. - -**Every structural claim about this change must be MEASURED before it is written.** T01.j's Result asserted three times that a change had simplified or shrunk something, and all three were contradicted by `wc -l` or `git show --numstat`. If you claim a condition was removed, paste the numstat. - -**An independent adversarial review is part of this task.** - -## Progress - -## Result