diff --git a/CHANGELOG.md b/CHANGELOG.md index 30c99d1..26eb103 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,19 @@ ## [Unreleased] -### TypeScript SDK — Breaking (npm package `@freenetorg/freenet-stdlib`; next release must be 0.4.0, not a patch) +### TypeScript SDK 0.4.0 — Breaking (npm package `@freenetorg/freenet-stdlib`) The npm package is versioned separately from the Rust crate. This release changes runtime behavior for existing callers, so on a 0.x line it takes a minor bump: **0.3.0 -> 0.4.0**, not 0.3.1. +Upgrading from npm: 0.3.0 and earlier matched responses to requests by *arrival +order alone*, with no correlation of any kind, so concurrent requests for +different contracts could resolve into each other's promises. That is what this +release fixes. Applications that serialised their requests to work around it can +stop doing so **for requests on different contracts**; see "Known limitation" +below before relaxing it for concurrent requests on the *same* contract. + - **Host responses are now correlated to requests by contract key.** `FreenetWsApi` previously resolved the *oldest* pending request of a type with whatever response arrived, with no correlation at all. Two concurrent @@ -46,6 +53,44 @@ minor bump: **0.3.0 -> 0.4.0**, not 0.3.1. names. An error naming no pending contract still fails everything, since it may be connection-wide and must not leave callers waiting out the timeout. +#### Known limitation: two requests for the SAME contract key + +Correlation is by contract key, because the key is the only identifying field a +`ContractResponse` carries. Two requests for the *same* key are therefore +indistinguishable, and one case remains open, reported as +[#96](https://github.com/freenet/freenet-stdlib/issues/96): + +Giving up on a request locally does not stop the node working on it — the SDK +sends nothing to cancel the operation — so a request that hit +`REQUEST_TIMEOUT_MS` can still be answered afterwards. If the caller has retried +the same contract by then, that late answer matches the retry exactly and +settles it. The retry resolves with a result fetched for a request its caller +already abandoned, and its own answer is later dropped against an empty queue. +Mostly this means staler state for the right contract, but not always: a retry +issued with `fetchContract: true` can be settled by an earlier response that +carries no contract. + +**This release does not fix that, deliberately.** A client-side fence was +attempted and withdrawn: with no request id on the wire, the SDK cannot tell a +late answer from the retry's own answer, so any rule that drops "the next +response for this key" is as likely to drop the retry's answer as the ghost's. +Doing so hangs that retry until its own timeout, which mints another +indistinguishable case — a self-sustaining chain of spurious timeouts against a +contract the node is serving correctly. That is a worse failure than the +mis-delivery it was meant to prevent. The evidence is on +[#105](https://github.com/freenet/freenet-stdlib/pull/105). + +The real fix is a client-generated request id echoed in every terminal response, +which makes correlation exact and this whole class of problem unreachable. That +is a wire-protocol change spanning freenet-core, tracked in +[#106](https://github.com/freenet/freenet-stdlib/issues/106). + +**Until then**, an application that issues concurrent requests for the same +contract key, or retries one after a timeout, should treat a response as +"an answer for this contract" rather than "the answer to this call": re-check +whatever the result is used for, and prefer idempotent retries. Requests for +*different* contracts are correlated correctly and need no such care. + ### Breaking (next release must be 0.9.0, not a patch) - **`DelegateRequest::RegisterDelegateWithPredecessors`** removed (added in 0.8.4). freenet-core's node-side handler for this request was disabled in diff --git a/typescript/package.json b/typescript/package.json index 0fe20d2..1f28e8c 100644 --- a/typescript/package.json +++ b/typescript/package.json @@ -1,6 +1,6 @@ { "name": "@freenetorg/freenet-stdlib", - "version": "0.3.0", + "version": "0.4.0", "description": "Freenet standard library and utils", "main": "dist/src/index.js", "types": "dist/src/index.d.ts", diff --git a/typescript/tests/correlation.test.ts b/typescript/tests/correlation.test.ts index 9cf2d22..6d5229b 100644 --- a/typescript/tests/correlation.test.ts +++ b/typescript/tests/correlation.test.ts @@ -673,3 +673,96 @@ describe("request timeout", () => { expect((await putB).key.encode()).toEqual(ENCODED_B); }); }); + +/** + * The residual that key-based correlation cannot close, pinned as a KNOWN + * LIMITATION rather than as desired behaviour. + * + * Two requests for the same contract key are identical on the wire — the key is + * the only identifying field a `ContractResponse` carries — so a late answer to + * a request the caller has already abandoned matches a retry for that key + * exactly. Reported as issue 96. + * + * A client-side fence for this was attempted and withdrawn (PR 105). With no + * request id, "drop the next response for this key" cannot tell the ghost's + * answer from the retry's own, and dropping the retry's answer hangs it until + * its own timeout — which mints another indistinguishable case, chaining + * spurious timeouts against a contract the node is serving correctly. That is + * worse than the mis-delivery it removes. + * + * These tests therefore assert what the SDK ACTUALLY DOES today. They are + * expected to be inverted, not deleted, by the wire-level request id that fixes + * this properly (issue 106). + */ +describe("known limitation: same-key requests are indistinguishable", () => { + const RESIDUAL_WS_URL = "ws://localhost:1243/contract/command/"; + let server: Server; + + beforeEach(() => { + server = new Server(RESIDUAL_WS_URL); + }); + + afterEach(() => { + jest.useRealTimers(); + server.clients().forEach((c) => c.close()); + server.close(); + }); + + async function connectResidual(): Promise { + const api = new FreenetWsApi(new URL(RESIDUAL_WS_URL), makeHandler()); + await settle(); + return api; + } + + test("a late answer to a timed-out get settles a retry for the same key", async () => { + const api = await connectResidual(); + + jest.useFakeTimers(); + const abandoned = api + .get(new GetRequest(new ContractKey(KEY_A), false)) + .catch((e) => e); + jest.advanceTimersByTime(31_000); + expect(((await abandoned) as Error).message).toEqual("Request timeout"); + + // The node is still working on the abandoned request; nothing cancelled it. + const retry = api.get(new GetRequest(new ContractKey(KEY_A), false)); + jest.useRealTimers(); + + // The abandoned request's answer arrives. It matches the retry's key + // exactly, and nothing on the wire distinguishes the two requests. + server + .clients() + .forEach((c) => c.send(getResponseFor(KEY_A, [0x01]))); + + // KNOWN LIMITATION: the retry settles with the abandoned request's answer. + // Invert this assertion once responses carry a request id. + expect((await retry).state).toEqual([0x01]); + }); + + test("a response for a DIFFERENT key never settles the wrong request", async () => { + const api = await connectResidual(); + + jest.useFakeTimers(); + const abandoned = api + .get(new GetRequest(new ContractKey(KEY_A), false)) + .catch((e) => e); + jest.advanceTimersByTime(31_000); + await abandoned; + + const other = api.get(new GetRequest(new ContractKey(KEY_B), false)); + jest.useRealTimers(); + + // The bound that DOES hold: the abandoned request's answer cannot reach a + // request for another contract. This is the half #94 fixed and the half + // that carried the real user-visible harm. + server + .clients() + .forEach((c) => c.send(getResponseFor(KEY_A, [0x01]))); + expect(await statusOf(other)).toEqual("pending"); + + server + .clients() + .forEach((c) => c.send(getResponseFor(KEY_B, [0x02]))); + expect((await other).state).toEqual([0x02]); + }); +});