chore(typescript)!: release 0.4.0 and pin the same-key correlation residual - #105
Conversation
…idual Ships the response-correlation work from #94, which has been sitting unreleased since 2026-08-22 while npm's published latest (0.3.0, from 2026-06-27) still matches responses to requests by arrival order alone. That is the change with the real user-visible harm behind it, and it is reaching nobody until this goes out. Both behavioral changes in this release land together, so on a 0.x line this is a minor bump rather than a patch. Also records what #94 does NOT fix. Correlation is by contract key, so two requests for the SAME key stay indistinguishable: a request that times out is not cancelled node-side, and its late answer matches a retry for that key exactly. Two tests pin this -- one asserting the residual as it actually behaves today, one asserting the bound that does hold (an answer never crosses to a different contract). The first is meant to be inverted, not deleted, once responses carry a request id. A client-side fence for the residual was attempted and withdrawn rather than shipped; see the PR for the evidence. With no request id the SDK cannot tell a late answer from the retry's own, so dropping "the next response for this key" drops the retry's answer as readily as the ghost's, hanging it until its own timeout and minting another indistinguishable case. Two facts from freenet-core make it worse: the session actor delivers at most one response per (transaction, client), so a host Error is terminal and no success follows it; and an operation can die without emitting a client result at all. A fence built on "the answer will still arrive" is therefore unbacked. The real fix is a request id on the wire, filed as #106. Refs #96 [AI-assisted - Claude]
9a4abda to
7f83317
Compare
sanity
left a comment
There was a problem hiding this comment.
Comprehensive PR Review: #105
Summary
- Review tier: Full (response correlation is a correctness-critical surface)
- Reviewers run:
code-first,skeptical,testing,big-picture(four independent Claude lenses, each blind to the others) + Codex (codex review --base origin/main) as the external non-Claude pass - Reviewed HEAD at time of findings:
9a4abda— the version that contained the debt fence - Current HEAD:
7f83317
Outcome: the reviewed change was withdrawn, not patched
The review found a defect that five of five reviewers independently converged on, and it was fatal to the approach rather than fixable within it. Rather than iterate, the mechanism was removed and the PR rescoped to the release plus a documented limitation.
The finding
The fence recorded a "debt" when a request was abandoned, letting the next response for that key settle the debt instead of a request queued afterwards. The SDK cannot tell a late answer from the retry's own answer — same message shape, same key — so whichever arrives first is absorbed. The likely ordering is the unhelpful one: the abandoned request timed out because it was slow, while the retry may be served quickly.
t=0 get(K) sent, slow
t=30 times out -> debt(K)
t=31 caller retries get(K)
t=35 the RETRY's own answer arrives -> absorbed by debt(K), dropped
t=61 retry times out -> debt(K) again ... repeats
Self-sustaining spurious timeouts against a contract the node is serving correctly — worse than the mis-delivery being fenced. The 240s expiry intended as the safety valve never fires, because each debt is spent long before it expires. Reproduced with a scratch test before acting (RETRY OUTCOME: PENDING (eaten)), rather than accepting the reviewers' reading on trust.
Two further findings made the approach unsafe in principle, not just in this shape — both traced to freenet-core and verified:
handle_result_deliveryremoves the transaction entry on first delivery, so a client receives at most oneHostResponseper(transaction, client). A hostErroris therefore terminal, andabandonOnRejectrecorded debts that could never be paid. (Codex P1; skeptical lens, with the core citation.)- An operation can die emitting no client result at all — result-router channel dropped under saturation,
pending_resultsLRU eviction. So "the answer will still arrive" is not guaranteed, and a debt is an unbacked claim.
Also found and accepted:
- Unbounded growth of the debt map —
abandon()never pruned, contradicting its own doc comment. (Codex P2; skeptical; code-first.) - The test suite did not pin the mechanism. The fake→real timer switch moves
Date.now()backwards ~31s, soABANDONED_RESPONSE_GRACE_MS = 0would have passed all eleven tests. The grace window — the entire safety argument — was unconstrained in the direction that mattered. (testing lens.) - Two tests were vacuous: "close rejects every pending request exactly once" was unfalsifiable (promise semantics guarantee it), and "a stale connection cannot settle its replacement's promises" was guaranteed by object separation. Both removed with the fence.
Closes #96over-claimed and the CHANGELOG's "you can stop serialising" advice was unsafe as written. Both corrected: nowRefs #96, and the advice is scoped to different-contract concurrency. (big-picture.)
What ships now
Source is byte-identical to origin/main — the committed diff is CHANGELOG.md, typescript/package.json, and additive tests only. The shipped behaviour is exactly #94, which was reviewed and merged on 2026-08-22.
- 0.4.0 release, so #94 finally reaches npm (published latest is still 0.3.0 from 2026-06-27).
- The residual is pinned by two tests: one asserting it as it behaves today (meant to be inverted, not deleted, by a request id), one asserting the bound that does hold.
- Real fix tracked in #106.
Verdict
State: Ready to Merge
HEAD SHA reviewed: 7f83317
Light re-check rather than a full re-review, justified because the runtime source reverted to already-reviewed origin/main content — the risky mechanism was removed, not reworked, and what remains carries no logic. 78 tests pass.
[AI-assisted - Claude]
#83) `@freenetorg/freenet-stdlib` <= 0.3.0 matched host responses to pending request promises by ARRIVAL ORDER, with no correlation at all: the Nth `GetResponse`/`NotFound` to arrive settled the Nth pending `get()`, whichever contract it was for. freenet-core drives each contract operation on its own task and publishes results as they complete, so responses arrive in completion order — the mismatch was reachable in practice, not theoretical. Raven worked around it by serialising every GET behind a promise chain (`getChain` / `serializedGet`) so only one was ever in flight. That was load-bearing: our shard flows use a GET as an existence probe and PUT when it rejects, so a swapped answer meant a shard was never instantiated (feed empty) or a like was silently lost. The cost was real latency — every GET waited out the previous one, including the full 30 s stdlib timeout of a hung GET. freenet-stdlib 0.4.0 correlates responses to requests by contract key (freenet/freenet-stdlib#105), so a response for contract A can no longer settle a pending request for contract B, and an unmatched response is dropped rather than mis-delivered. The serialisation is obsolete. - Bump `@freenetorg/freenet-stdlib` ^0.2.0 -> ^0.4.0. On a 0.x line `^0.2.0` means `>=0.2.0 <0.3.0`, so this is a major-equivalent bump. - Remove `getChain`; `serializedGet` becomes `boundedGet`, which issues the GET immediately. The caller-visible soft 8 s deadline is DELIBERATELY PRESERVED — callers still give up at 8 s rather than waiting out the stdlib's own 30 s timeout. - Replace the four `serializedGet` serialisation tests with three `boundedGet` tests pinning the new behaviour: concurrent GETs for different contracts both go out and settle independently, a stalled GET does not block later ones, and the soft deadline still releases the caller. All three were verified to fail against a mutation that reintroduces the removed behaviour. - Correct comments and the ADR whose stated rationale 0.4.0 invalidates. Not changed, deliberately: - 0.4.0 does NOT fix two concurrent requests for the SAME contract key (freenet/freenet-stdlib#96) — one response settles both, and a request abandoned at the soft deadline is not cancelled node-side, so a late answer can settle a same-key retry. Benign for every GET here: same contract, same answer, and the probes read only resolve-vs-reject (state reaches the app through the `onContractGet` handler, which routes by key itself). Documented at the call site and in the ADR. - All four `api.subscribe()` call sites already have rejection handlers, so 0.4.0's change (subscribe now awaits the host's confirmation and can reject, where before it resolved on send and never rejected) produces no unhandled rejection. Behaviour is left as-is; using the now-observable refusal to retry the global-index subscribe would be a separate improvement. Verified: `npm ci` (0.4.0 installed), `tsc --noEmit` clean, `svelte-check` 0 errors (14 pre-existing warnings, same as main), `vite build` succeeds, `vitest` 73/73 pass. Note the vitest suite needs `web/*_code_hash.txt` from `cargo make build-contracts`, per the `[tasks.test]` note in Makefile.toml. [AI-assisted - Claude] Claude-Session: https://claude.ai/code/session_01D2yvFcLd56ZaeoRi4mojcD
What this does
History of this PR — a fix was attempted and withdrawn
This PR originally added a client-side fence for the same-key case in #96: when a request was abandoned without an answer, record a "debt" for its key, and let the next response for that key settle the debt rather than a request queued afterwards.
It was withdrawn as unsound. Review (four Claude lenses plus Codex) converged on the same defect, and I reproduced it before acting:
The SDK cannot tell a late answer from the retry's own answer. They are the same message shape for the same key. Whichever arrives first is absorbed — and the likely ordering is the unhelpful one, because the abandoned request timed out precisely because it was slow, while the retry may be served quickly. So the fence drops the retry's own answer, the retry hangs until its own 30s timeout, and that mints another indistinguishable case:
A self-sustaining chain of spurious timeouts against a contract the node is serving correctly — worse than the mis-delivery it removes. The 240s expiry I added as the safety valve never fires, because each debt is spent long before it can expire. The test I wrote for that valve covered the case where no response arrives, which is exactly the case that cannot occur in this loop.
Two facts from freenet-core make any such fence unsafe in principle, not just in this shape:
HostResponseper (transaction, client) —handle_result_deliveryremoves the transaction entry on first delivery. So a hostErroris terminal; a fence assuming a success can still follow it records a claim that will never be paid.pending_resultsLRU eviction). "Eventually answered" is not guaranteed.So a debt is an unbacked claim on a response that may never come. The withdrawn commit is
ec9be1fif anyone wants to read it.What ships instead
The CHANGELOG gains a Known limitation section stating the residual plainly, including the case that is not merely "staler state": a retry issued with
fetchContract: truecan be settled by an earlier response carrying no contract.Two tests in
tests/correlation.test.ts:a late answer to a timed-out get settles a retry for the same key— asserts the residual as it actually behaves today. This is meant to be inverted, not deleted, once responses carry a request id.a response for a DIFFERENT key never settles the wrong request— asserts the bound that does hold, which is the half fix(typescript)!: correlate host responses to requests by contract key #94 fixed and the half that carried the user-visible harm.The upgrade note is scoped accordingly: applications that serialised requests can stop doing so for different contracts, with the same-key caveat spelled out. (An earlier draft said this unconditionally, which review correctly flagged as unsafe advice.)
The real fix
#106 — a client-generated request id echoed in every terminal response. Appending an optional field to the flatbuffers response tables is backward and forward compatible, and the SDK can fall back to key matching against older nodes during rollout. freenet-core already tracks a
RequestIdinternally and drops it beforeClientEventsProxy::send, so the correlator exists and is simply not exposed.Refs #96rather thanCloses #96— the reported failure mode is still reachable and the reporter should get the chance to disagree with this call.Testing
78 tests pass (76 pre-existing + 2 new). No test was removed or weakened; the diff is additive apart from the version bump.
Refs #96
[AI-assisted - Claude]