Repository navigation
Conversation
After a device slept or the network dropped, the pubsub client could silently stop receiving events: it gave up on unanswered subscriptions, kept half-open sockets around, and Chelonia never noticed the gap. The client now replaces the connection instead of dropping a wanted request, abandons dead sockets right away (ping/connection timeout, offline, wake-up from sleep) and exposes its connection state. Chelonia re-checks its subscriptions after every reconnection and periodically, syncs contracts whose published events don't come back, syncs on a publish or KV write conflict, and keeps the server time estimate correct across sleep. Includes an in-process fake pubsub server and relay for tests, plus docs for the new options, events and status. Fixes #121. See okTurtles/group-income#3176. Assisted-by: Crush:claude-opus-5-5
|
/review AI review started. The review job timed out. |
|
@corrideat if you haven't already please also run a local review with the code-review skill using Opus 5.5 🙏 |
|
/review {{kimi}} AI review started. |
Advanced AI Review
Click to expand reviewPreparing five read-only review workers in two batches. Review generated using |
|
Review by Opus 5.5 xhigh: DetailsCode ReviewBase: Context: okTurtles/libcheloniajs#125, "fix: recover lost pubsub subscriptions after sleep and dead sockets" (fixes #121). 1. 🔴
|
| Node | typeof CloseEvent |
npm test result |
|---|---|---|
| v22.23.1 | undefined |
547 tests, 516 pass, 25 fail (ReferenceError: CloseEvent is not defined) |
| v24.22.2 | function |
all pass |
Failing on Node 22: recovery.test.ts, dead-socket.test.ts, chelonia-pubsub-reconcile.test.ts, chelonia-publish-receipt.test.ts, and the whole of chelonia-kv-set-sync.test.ts and time-sync.test.ts.
The client already has a fallback (createCloseEvent, src/pubsub/index.ts:357-362). Use the same pattern in the harness, export it, and use it in recovery.test.ts:359:
// Node 22 has no global `CloseEvent` (added in Node 23)
export const createCloseEvent = (code: number, reason = '', wasClean = false): Event =>
typeof CloseEvent === 'function'
? new CloseEvent('close', { code, reason, wasClean })
: Object.assign(new Event('close'), { code, reason, wasClean })
const dispatchClose = (conn: FakeConnection, code: number, reason: string, wasClean: boolean) => {
conn.state = 'closed'
conn.socket.readyState = CLOSED
conn.socket.dispatchEvent(createCloseEvent(code, reason, wasClean))
}
// A connection attempt that fails (or is aborted): `error`, then close 1006
const failConnection = (conn: FakeConnection) => {
conn.state = 'closed'
conn.socket.readyState = CLOSED
conn.socket.dispatchEvent(new Event('error'))
conn.socket.dispatchEvent(createCloseEvent(1006))
}2. 🟡 A 409 during publish can sync a released contract and subscribe to it again
- Addressed
- Dismissed
src/internals.ts:958-963:
if (!entry.isFirstMessage()) {
const localHeight = sbp(this.config.stateSelector).contracts[contractID]?.height
if (localHeight == null || localHeight < entry.height()) {
await sbp('chelonia/private/in/sync', contractID, { force: true })
}
}For a message that isn't the first one, localHeight == null is true only when the contract state was removed. This can happen during the delay(randDelay) at line 951 (up to 1.5 s, outside the contract queue) if chelonia/contract/release runs. Then:
chelonia/private/in/syncwithforcedoes not checksubscriptionSet(internals.ts:774).syncContractadds a contract that is not subscribed tothis.pending(internals.ts:2089-2096), so the sync subscribes to it again. Nothing holds a reference to it, so nothing will release it.- If the contract was deleted for good (
contracts[id] === null),syncContractthrowsChelErrorResourceGone(internals.ts:2065-2067) and the publish rejects. Without the sync, the next loop would stop quietly at line 837 ("contract state has been removed").
The old condition (entry.height() === lastAttemptedHeight) could also sync in this case, so the race is not new. But the new == null clause selects this case on purpose. The two other sync paths that this PR adds both guard against it: ensurePublishedEventReceived (internals.ts:314, 321) and kv/set (chelonia.ts:3193).
The kv/set path also catches a failed sync and retries (chelonia.ts:3197-3203). Here, a failed sync (e.g. a network error in latestHEADInfo) goes to the catch at line 977 and fails the publish, even though attempts remain. Make the two paths agree:
if (!entry.isFirstMessage() && this.subscriptionSet.has(contractID)) {
const localHeight = sbp(this.config.stateSelector).contracts[contractID]?.height
if (localHeight == null || localHeight < entry.height()) {
try {
await sbp('chelonia/private/in/sync', contractID, { force: true })
} catch (e) {
console.warn(
`[chelonia] Error syncing ${contractID} after a publish conflict; retrying anyway: ${(e as Error)?.message || e}`
)
}
}
}3. 🟡 oldestPendingSince is wrong while disconnected, and covers channels that pending leaves out
- Addressed
- Dismissed
Client side. The docs say that since is set only when a request is sent on the current connection (src/pubsub/index.ts:73-76: "requests waiting for a connection don't count"; :79-83). But runWithRetry sets it when it creates the entry, before it checks the socket (src/pubsub/index.ts:436):
const instance: PendingOperation = { since: Date.now() }
client.pendingOperations.tSet(type, channelID, instance)
const send = () => {
if (client.socket !== socket || socket?.readyState !== WebSocket.OPEN) returnSo a sub() call in the connecting, reconnecting or offline state (for example from CONTRACTS_MODIFIED → chelonia/pubsub/update during a reconnect) gets a timestamp, although nothing was sent. getStatus() (index.ts:1118-1124) then reports a non-null oldestPendingSince while disconnected. docs/debugging.md:109-111 tells readers that "a pending list with an old oldestPendingSince means that the relay doesn't answer". After a long time offline, that diagnosis is wrong. The tests don't catch it: dead-socket.test.ts:437-472 calls sub() only while the socket is OPEN.
Set the time on the first real send:
const instance: PendingOperation = {}
// ...
// 3. Send logic
const payload = getPayload()
instance.since ??= Date.now()
socket.send(createRequest(type, payload))Chelonia side. chelonia/pubsub/status filters pending to the contracts Chelonia wants, but copies oldestPendingSince from the client unchanged (src/chelonia.ts:1643-1644). So pending: [] can come with a non-null oldestPendingSince. Either filter it (have getStatus() return the since of each pending channel), or say in docs/debugging.md and the ChelPubSubStatus comment (src/types.ts:473-474) that it covers all channels.
4. 🟡 onNextOpen() has no timeout, so a reconnection bug makes tests hang instead of fail
- Addressed
- Dismissed
src/pubsub/test-harness.ts:643-656 returns a promise that settles only on PUBSUB_RECONNECTION_SUCCEEDED. Tests call onNextOpen(client), then killClients(), then await it with no time limit (e.g. recovery.test.ts:215-220, and the reconnect tests in chelonia-pubsub-reconcile.test.ts). Reconnection is exactly what these tests check. If it breaks, the await never returns. Client retry timers keep the event loop alive, node --test has no default per-test timeout, and ci.yml sets no timeout-minutes. So CI hangs instead of showing a failed assertion, and captureConsole() hides the logs. waitFor already gives up after a timeout "so that assertions can report what went wrong".
When a caller's Promise.race loses (dead-socket.test.ts:89-93), the listener also stays registered and runs callback on a later open.
export function onNextOpen (
client: PubSubClient,
callback?: () => void,
timeout = 5000
): Promise<number> {
return new Promise((resolve, reject) => {
const listener = (target: PubSubClient) => {
if (target !== client) return
clearTimeout(timer)
sbp('okTurtles.events/off', PUBSUB_RECONNECTION_SUCCEEDED, listener)
callback?.()
resolve(Date.now())
}
const timer = setTimeout(() => {
sbp('okTurtles.events/off', PUBSUB_RECONNECTION_SUCCEEDED, listener)
reject(new Error(`onNextOpen: no open within ${timeout} ms`))
}, timeout)
sbp('okTurtles.events/on', PUBSUB_RECONNECTION_SUCCEEDED, listener)
})
}5. 🟡 publishReceiptTimeout docs link to the wrong section and describe the trigger wrongly
- Addressed
- Dismissed
docs/configure.md:84 ends with "See When a publish fails". That section (docs/contracts.md:676-702) is about HTTP rejections and does not mention this option. The option is documented in the new section ### Making sure a published event is processed (docs/contracts.md:704).
The same row says "If the pubsub client isn't subscribed to the contract … Chelonia syncs right away". The code syncs right away only when the client is neither subscribed nor subscribing (src/internals.ts:337-343). While a SUB is pending, it waits for the timeout; with publishReceiptTimeout: 0 it never syncs. docs/contracts.md:712-713 says this correctly, so the two pages disagree.
| `publishReceiptTimeout` | `number` | `5000` | How long (in ms) to wait, after the relay accepted an event published to a contract Chelonia is subscribed to, for that event to come back over pubsub before syncing the contract to process it (e.g. the socket is half-open). If the pubsub client is neither subscribed nor subscribing to the contract (or there is no pubsub client), Chelonia syncs right away instead. `0`, or anything that isn't a positive finite number, disables the timed check only. See [Making sure a published event is processed](./contracts.md#making-sure-a-published-event-is-processed). |6. 🟡 The new test files copy the same setup helpers
- Addressed
- Dismissed
The PR adds a shared harness, but the new test files still repeat its setup:
- Chelonia tests.
src/chelonia-kv-set-sync.test.tsrepeatssrc/chelonia-publish-receipt.test.tsalmost line for line: theEVENT_HANDLEDset (:37-41vs:35-39),_init/configure/connect(:43-57vs:63-77),localHeight,retainedContract()(:72-81vs:87-95),staleContract()(:85-98vs:99-112) and the/latestHEADinfo/<id>request count (:153, again at:297-299, and:59-60in the other file).chelonia-pubsub-reconcile.test.ts:68-80andtime-sync.test.tsrepeatisOpenand the configure + connect + wait-for-open steps. - Pubsub client tests.
src/pubsub/recovery.test.ts:12-52repeatssrc/pubsub/dead-socket.test.ts:23-85(URL, server, client list,before/afterEach/afterhooks,setup(),isOpen,subscribed).
Move them into src/pubsub/test-harness.ts and pass relay / pubsub in:
export const isOpen = (client: PubSubClient): boolean =>
client.socket?.readyState === WebSocket.OPEN
// Number of syncs of `contractID` among the relay's requests from position `from`
export const syncsOf = (relay: FakeRelay, contractID: string, from: number): number =>
relay.requests.slice(from).filter(({ path }) => path === `/latestHEADinfo/${contractID}`).length
// Configures Chelonia against `relay` and connects it to the fake server
export async function connectChelonia (
relay: FakeRelay,
config: Record<string, unknown> = {}
): Promise<PubSubClient> {
await sbp('chelonia/configure', {
connectionURL: 'http://relay.test',
fetch: relay.fetch,
connectionOptions: { minReconnectionDelay: 20 },
...config
})
const pubsub = sbp('chelonia/connect') as PubSubClient
if (!(await waitFor(() => isOpen(pubsub)))) throw new Error('not connected')
return pubsub
}
// A contract that Chelonia has synced and `pubsub` is subscribed to
export async function retainedContract (
relay: FakeRelay,
pubsub: PubSubClient
): Promise<TestContract> { /* … */ }7. ⚪ A request that is never answered makes the client reconnect (and re-sync everything) forever
- Addressed
- Dismissed
After the retry budget runs out, the client always replaces the socket (src/pubsub/index.ts:449-457). The next open sets failedConnectionAttempts = -1 (index.ts:770), so backoff never grows and maxRetries never applies to this loop. If the relay never answers one request (a relay bug for one channel, or a relay that ignores kv_filter), the cycle repeats every 20–30 s with the defaults. The old code dropped the request ("Giving up").
Each cycle clears all subscriptions. Each SUB ack then runs Chelonia's subscription-succeeded handler, which force-syncs the contract (src/chelonia.ts:1135-1153), and PUBSUB_RECONNECTION_SUCCEEDED runs chelonia/kv/_onReconnect. So one bad request makes the client re-sync every contract about every 25 s.
Replacing the socket is a deliberate choice; only the missing limit looks unplanned. One option: limit consecutive replacements for unanswered requests, then drop the request and let the periodic reconcile ask again:
if (attemptNo++ > options.maxOpRetries) {
if (client.unansweredReplacements >= options.maxRetries) {
console.warn(`[pubsub] Giving up ${type} for ${channelID} after replacing the connection ${client.unansweredReplacements} times`)
client.pendingOperations.tDelete(type, channelID)
return
}
client.unansweredReplacements++
abandonSocket(client, socket, `no response to ${type} for ${channelID}`)
return
}Reset client.unansweredReplacements = 0 in the RESPONSE_TYPE.OK / RESPONSE_TYPE.ERROR handlers.
8. ⚪ With reconnectOnDisconnection: false, one unanswered request now closes the client for good
- Addressed
- Dismissed
abandonSocket (src/pubsub/index.ts:456) runs the close handler. That handler reconnects only if client.shouldReconnect && client.options.reconnectOnDisconnection (index.ts:662). With reconnectOnDisconnection: false (an existing option, used in index.test.ts), one slow SUB/UNSUB/KV_FILTER now closes the socket, nothing replaces it, and every other subscription is lost. Before, the client dropped only that request. The option is documented as being about server-side disconnections, and the comment at index.ts:100-103 promises that the connection is "replaced".
if (attemptNo++ > options.maxOpRetries) {
if (!options.reconnectOnDisconnection) {
// Nothing would replace the connection: keep it and drop the request
console.warn(`[pubsub] Giving up ${type} for channel`, channelID)
client.pendingOperations.tDelete(type, channelID)
return
}
abandonSocket(client, socket, `no response to ${type} for ${channelID}`)
return
}9. ⚪ The ping timer restarts only on PING, but the docs say "any message"
- Addressed
- Dismissed
The new text says the timeout is about any message: src/pubsub/index.ts:87-90 ("How long to wait for the next message"), :392-393, the createClient JSDoc (:488-489) and docs/debugging.md:139 ("when nothing arrived for pingTimeout ms"). But armPingTimeout runs only from open and from the default PING handler (index.ts:828). The wall-clock watchdog uses lastMessageAt, which every message updates (index.ts:693). So the two checks disagree, and a consumer that replaces the PING handler through options.messageHandlers never restarts the timer.
Restart the timer where lastMessageAt is set, and remove the call from the PING handler:
client.lastMessageAt = Date.now()
armPingTimeout(client)Or change the docs to say "the next ping".
10. ⚪ Longer retry delays now keep Node processes alive after destroy()
- Addressed
- Dismissed
The old cap Math.min(200, minDelay) was removed (src/pubsub/index.ts:467-470). With the defaults, the last retry timer can now wait up to about 10 s, and destroy() does not cancel it. The new test has to work around this (src/pubsub/dead-socket.test.ts:439-441: "Retry timers are not cleared on destroy(), so a longer interval would keep the process alive."). The timer does nothing when it fires, but it keeps the Node event loop alive, which affects consumers' test suites. src/chelonia.ts:1369 already uses unref for the same problem:
const retryID = setTimeout(send, delay)
// Node.js-specific: a retry must not keep the process alive (no-op in browsers)
;(retryID as unknown as { unref?: () => void }).unref?.()Then the workaround comment in the test can go.
11. ⚪ chelonia/pubsub/status counts a contract as subscribed while its UNSUB is in flight
- Addressed
- Dismissed
The client keeps a channel in subscriptionSet until the UNSUB is acknowledged (src/pubsub/index.ts:910-914), and getStatus() does not report pending UNSUBs (index.ts:1112-1131). If a contract is released and retained again before the UNSUB ack, chelonia/pubsub/status (src/chelonia.ts:1635-1645) reports it as neither missing nor pending, and chelonia/pubsub/update puts it in leaveSubscribed (chelonia.ts:1599). When the ack arrives, the client drops the channel. The contract stays lost until the next periodic reconcile (up to 60 s), and the new diagnostic says all is well.
The pubsub/update part is older than this PR, but the new status selector repeats it. Report pending UNSUBs from getStatus() and count them as missing:
const status = client.getStatus()
const unsubscribing = new Set(status.unsubscribing)
const subscribed = new Set(status.subscribed.filter((id) => !unsubscribing.has(id)))A full fix also needs sub() to cancel a pending UNSUB and send a SUB (src/pubsub/index.ts:1204-1211).
12. ⚪ Duplicated "positive finite number" check; the publishReceiptTimeout: 0 test can't catch a fallback to the default
- Addressed
- Dismissed
The same check is written twice:
src/chelonia.ts:1347-1351:typeof reconcileInterval === 'number' && reconcileInterval > 0 && Number.isFinite(reconcileInterval)src/internals.ts:346:typeof timeout !== 'number' || !(timeout > 0) || !Number.isFinite(timeout)
Number.isFinite already returns false for non-numbers, so the typeof part is redundant. One helper removes the copy:
// A positive finite number of ms, or `undefined` (disabled)
export const positiveDelay = (value: unknown): number | undefined =>
Number.isFinite(value) && (value as number) > 0 ? value as number : undefinedSeparately, src/chelonia-publish-receipt.test.ts:235-244 waits only sleep(500) before it asserts that no sync happened. If a later change treated 0 as "unset" and fell back to 5000, the test would still pass (the timer is unref()'d, internals.ts:350). Unit-test the helper with 0, -1, NaN, Infinity, undefined and 300 instead.
13. ⚪ The reconcile-timer test cannot detect a leaked timer
- Addressed
- Dismissed
src/chelonia-pubsub-reconcile.test.ts:313-327 checks "replaces … and stops it once the client is destroyed" only by counting chelonia/pubsub/update calls. A leaked interval cannot change that count: the interval body reconciles only when client.socket?.readyState === WebSocket.OPEN (src/chelonia.ts:1363), and a destroyed client has socket = null (src/pubsub/index.ts:1106). So removing clearInterval(this.pubsubReconcileIntervalID) (chelonia.ts:1344) or the !client.shouldReconnect self-stop (chelonia.ts:1356-1361) would not make the test fail. Watch the timer itself:
it('replaces the periodic reconcile on chelonia/connect and stops it once the client is destroyed', async (t) => {
const INTERVAL = 100
const started = t.mock.method(globalThis, 'setInterval')
const cleared = t.mock.method(globalThis, 'clearInterval')
// The reconcile timers are the only intervals of INTERVAL ms
const timers = () => started.mock.calls
.filter(({ arguments: args }) => args[1] === INTERVAL)
.map(({ result }) => result)
const isCleared = (timer: unknown) =>
cleared.mock.calls.some(({ arguments: args }) => args[0] === timer)
await connect(INTERVAL)
await connect(INTERVAL)
const [first, second] = timers()
assert.ok(isCleared(first), 'the timer of the first client is still running')
assert.ok(!isCleared(second))
pubsub.destroy()
assert.ok(
await waitFor(() => isCleared(second), { timeout: INTERVAL * 3 }),
'the timer outlived the client'
)
})14. ⚪ The Giving up assertions check a log line that no longer exists
- Addressed
- Dismissed
This PR removed the [pubsub] Giving up … log from runWithRetry. No code in src/ logs it any more (the only match is a thrown error in utils.ts:710). These assertions check wording rather than behaviour:
src/pubsub/recovery.test.ts:222(obs.giveUps = logs.grep(/Giving up/, tOpen).length),:243,:251src/pubsub/recovery.test.ts:343
"Not giving up" means the SUBs stay pending. Check that directly:
await sleep(OUTAGE)
// The old client dropped these SUBs after its retries; they must stay wanted
obs.stillPending = ids.every((id) => client.pendingOperations.tHas(REQUEST_TYPE.SUB, id)) it('replaces the dead connection instead of giving up on the channels', () => {
assert.ok(obs.stillPending, 'a SUB was dropped instead of kept for the next connection')
assert.ok(obs.abandoned >= 1, 'the dead socket was not abandoned')
})Delete line 343. At line 243, report obs.abandoned instead of obs.giveUps.
15. ⚪ Missing assertions for UNSUB/KV_FILTER budgets and for the 412 path
-
Addressed
-
Dismissed
-
The SUB test (
src/pubsub/recovery.test.ts:339-342) checks 5 sends and theAbandoning socket: no response to sub for ulog. The UNSUB test (:367-383) and the KV_FILTER test (:385-397) only check that the connection changed. They still pass if the request is abandoned after one try or with the wrong reason. Add the same two checks:assert.equal(first.frames.filter(({ msg }) => msg.type === REQUEST_TYPE.UNSUB).length, 5) assert.equal(newLogs.grep(/Abandoning socket: no response to unsub for w$/).length, 1)
-
chelonia/kv/setsyncs only onresponse.status === 409(src/chelonia.ts:3190-3195). No test inchelonia-kv-set-sync.test.tsshows that a 412 (stale etag) does not sync, so the guard could widen without a test failing:it('does not sync on a 412 (a stale etag, not a stale height)', async () => { const { contract } = await staleContract() relay.onRequest = ({ method, path }) => { if (method === 'POST' && path.startsWith('/kv/')) { return new Response('', { status: 412, headers: { 'x-cid': '""' } }) } } const result = await kvSet(contract, { messageHash: 'x' }, { maxAttempts: 2 }) assert.ok(result.error instanceof ChelErrorKvMaxAttempts, describeError(result.error)) assert.deepEqual(result.statuses, [412, 412]) assert.equal(result.syncs, 0) })
16. ⚪ The connectionOptions.* docs row overstates two things
- Addressed
- Dismissed
docs/configure.md:82:
- "Every other field is passed to the pubsub client as is."
chelonia/configuremergesconnectionOptionswith turtledashmerge, which deep-clones objects throughJSON.parse(JSON.stringify(...)). SoInfinitybecomesnulland functions are dropped. Also,chelonia/connectoverrideshandlersandmessageHandlersfrom its own argument (src/chelonia.ts:1131-1157). This is older than the PR, but the new wording promises the opposite. - "the client replaces the connection and sends the request again." This is true only for SUB. Pending UNSUBs and KV filters are cleared on close (
src/pubsub/index.ts:659-660); the filter goes out with the next SUB.
| `connectionOptions.*` | pubsub client `Options` | see `src/pubsub/index.ts` | Every other JSON-serializable field is passed to the pubsub client (`chelonia/configure` turns `Infinity` into `null`; event handlers go in the argument of `chelonia/connect`), e.g. `pingTimeout` (…), `opRetryInterval` (`2000`) and `maxOpRetries` (`4`). A SUB/UNSUB/KV filter request that gets no answer after `maxOpRetries` retries (about 10 to 15 x `opRetryInterval` with the defaults) is never dropped: the client replaces the connection and sends the SUBs (with their KV filters) again on the new one; an UNSUB is then complete, since the server drops the old connection's subscriptions. Read by `chelonia/connect`. |17. ⚪ Harness fidelity: waitFor clock, server.kv() filters, close() checks
-
Addressed
-
Dismissed
-
waitForuses a patched clock.waitForsets its deadline fromperformance.now()(src/pubsub/test-harness.ts:631), whichemulateSleep({ monotonic: true })patches (:697). Awake(ms)during awaitFormoves its deadline byms(5 minutes intime-sync.test.ts). No current test does this, but it is an easy trap. Capture the real function at import time:// Not affected by `emulateSleep({ monotonic: true })` const realPerformanceNow = performance.now.bind(performance)
-
server.kv()ignores KV filters. The fake records each connection's filter (test-harness.ts:150-177), butserver.kv()(:349-351) notifies every subscriber. A future KV-filter test would see behaviour the real server doesn't have. No test calls it; delete it or apply the filter:kv (channelID: string, key: string, data: unknown, cid?: string) { for (const conn of subscribersOf(channelID)) { const filter = conn.kvFilters.get(channelID) if (!filter || filter.includes(key)) { deliver(conn, { type: 'kv', channelID, key, data, cid }) } } },
-
FakeWebSocket.close()accepts any code and reason (test-harness.ts:238-256). The real API throwsInvalidAccessErrorfor codes other than 1000 or 3000–4999, andSyntaxErrorfor a reason over 123 UTF-8 bytes. So the fallbackcatchinabandonSocket(src/pubsub/index.ts:380-387) never runs in tests, and a bad close code would pass:close (code?: number, reason?: string) { if (code !== undefined && code !== 1000 && !(code >= 3000 && code <= 4999)) { throw new DOMException(`Invalid close code ${code}`, 'InvalidAccessError') } if (reason !== undefined && new TextEncoder().encode(reason).length > 123) { throw new DOMException('Close reason too long', 'SyntaxError') } // …
18. ⚪ Dead code and copied logic in the harness
-
Addressed
-
Dismissed
-
Unused: the
server.latencygetter (test-harness.ts:262),relay.requestsTo()(:602-605), andFakeConnection.createdAt/openedAt(:64-65, written but never read). -
The
subandkv_filtercases repeat the same set-or-delete of the filter (:153-157and:170-174), and the switch uses literal protocol strings ('sub','unsub','kv_filter','ok','ping','entry','kv', …). ImportREQUEST_TYPE,RESPONSE_TYPEandNOTIFICATION_TYPEfrom./index.jsso that the fake follows the client if a value changes:const setKvFilter = (conn: FakeConnection, channelID: string, kvFilter: unknown) => { if (Array.isArray(kvFilter)) conn.kvFilters.set(channelID, kvFilter) else conn.kvFilters.delete(channelID) }
19. ⚪ Small issues in chelonia-kv-set-sync.test.ts
-
Addressed
-
Dismissed
-
Wrong comment.
:51-52says "A lost subscription stays lost" next topubsubReconcileInterval: 0. ButCONTRACTS_MODIFIEDstill runschelonia/pubsub/update(src/chelonia.ts:1301), so every laterretainedContract()re-subscribes the contracts that earlier tests made stale. The assertions still pass becausesyncsis filtered by contract ID.// No periodic reconcile: a contract lost with `pubsub.unsub()` stays // lost until the next `chelonia/pubsub/update` (e.g. the next retain) pubsubReconcileInterval: 0,
-
Repeated
try/finally.:214-221,:234-241and:252-260each resetrelay.onRequestin afinally. OneafterEach(() => { relay.onRequest = undefined })does the same and covers new tests. -
queuedSettest.encryptionKeyName: '#cek'(:287) names a key thatcreateTestContract()doesn't define (only#cskand#sak), so the value is written unencrypted anyway. Remove it. The manualPromise.race+setTimeout(:290-295) does what node:test's per-test{ timeout: 5000 }option does.
20. ⚪ Simplify the SUB acknowledgement lookup
- Addressed
- Dismissed
src/pubsub/index.ts:889-895 calls tHas, then tGet(...) ?? {} on the same key, then tDelete. Entries are always objects, so one tGet is enough:
case REQUEST_TYPE.SUB: {
const pending = client.pendingOperations.tGet(REQUEST_TYPE.SUB, channelID)
if (pending) {
const elapsed = pending.since === undefined ? 0 : Date.now() - pending.since
if (elapsed > client.options.opRetryInterval) {
console.info(`[pubsub] Late ACK: subscribed to ${channelID} ${elapsed} ms after the request`)
}
client.pendingOperations.tDelete(REQUEST_TYPE.SUB, channelID)21. ⚪ Test comments and titles refer to labels and files that aren't in the repo
-
Addressed
-
Dismissed
-
3175-report/..."Port of" headers:src/pubsub/dead-socket.test.ts:4,src/pubsub/recovery.test.ts:192. -
CL-nlabels, which nothing in the repo defines. Several are indescribe/ittitles, so they appear in every test report:CL-1,CL-2,CL-10:src/pubsub/recovery.test.ts:240, 246, 250, 255, 304, 311, 318, 400CL-3,CL-4,CL-5:src/pubsub/dead-socket.test.ts:1-3and itsdescribetitlesCL-6,CL-10:src/chelonia-pubsub-reconcile.test.ts:2, 129, 210, 235, 260CL-7:src/chelonia-publish-receipt.test.tsdescribetitlesCL-8:src/chelonia-kv-set-sync.test.ts:3, 163, 192, 225, 264, 277CL-9:src/time-sync.test.ts:2
Remove them, or replace them with a link to okTurtles/group-income#3176 / #121.
22. ⚪ AGENTS.md now holds implementation detail
- Addressed
- Dismissed
The new "Pubsub recovery model" section (AGENTS.md:192-214) describes internals: abandonSocket, close code 4001, maxOpRetries, the wall-clock ping check, and which selector syncs when. The test-harness paragraph (AGENTS.md:385-392) lists helper functions and how they work. AGENTS.md should be a table of contents plus general rules; details belong in code comments and docs. Most of this text already exists in docs/debugging.md and in src/pubsub/index.ts comments, so the copies will drift. Suggested replacement:
### Pubsub recovery
Chelonia must stay subscribed (in `src/pubsub/index.ts`) to every contract in its
`subscriptionSet`. For how the client and Chelonia recover lost subscriptions and dead
sockets, see [docs/debugging.md](docs/debugging.md#pubsub-connection).Pubsub and network tests use the in-process fakes in `src/pubsub/test-harness.ts` (there is
no WebSocket server in the repo). Example: `src/chelonia-publish-receipt.test.ts`.23. ⚪ The ESM build compiles and ships the test harness
- Addressed
- Dismissed
tsconfig.cjs.json:21-24 excludes the harness from the CJS build, but tsconfig.json:22-23 includes all of src/**/*.ts. So npm run build:esm writes dist/esm/pubsub/test-harness.mjs and .d.mts, and with no files list or .npmignore they go into the npm package. The existing src/test-utils.ts has the same treatment (dist/esm/test-utils.mjs is tracked), so the impact is low. If test-only code should not ship, exclude both from the ESM build too.
24. ⚪ Ambiguous sentence in docs/contracts.md
- Addressed
- Dismissed
docs/contracts.md:709-710: "so for a contract Chelonia is subscribed to it checks after every successful publish" first reads as "subscribed to it". Add a comma:
half-open, that never happens, so for a contract Chelonia is subscribed to,
it checks after every successful publish:
Another Opus 5.5 high reviewCode ReviewBase: Context: #121, items CL-1 to CL-10. The branch covers every item. On Node 26, 1. 🔴 The new tests need a global
|
Most recovery tests failed on CI's Node 22, which has no global CloseEvent; the test harness now creates close events itself. The helpers that were copied across test files now live in the harness, and its fake socket rejects what a real one rejects. Assertions that could never fail now check what they claim, and the KV write paths that had no tests now have some. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
When the relay rejected a published event as conflicting, Chelonia synced the contract to catch up, even if the app had released or deleted it while the request was in flight. That sync subscribed to the released contract again, and failed on a deleted one. Chelonia now only syncs contracts that it still wants and has local state for. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
A subscription or KV filter that the relay never answered made the client replace the connection, and every new connection was opened after the shortest delay. A slow or overloaded relay then got a storm of reconnections and resyncs. Replacements in a row now wait longer each time, like failed connection attempts, until every pending subscription and KV filter has been answered. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
- The oldest pending subscription in the status only counts requests that were actually sent, and Chelonia's status only the ones that Chelonia wants, so a slow connection attempt isn't reported as an unanswered request. - Any message from the server, not only a ping, now shows that the connection works, as the wake-up check already assumed. - Coming back online no longer restarts a client that just gave up reconnecting. - The answers to a subscription's retries are logged at debug level, not as unexpected at info level. - The client no longer needs a global WebSocket before it connects, and the docs say when the first connection state event fires. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
The server time estimate is based on the moment the reply arrived, but the reference point for noticing later clock jumps was taken after its body was read. A sleep in between went unnoticed, and the estimate stayed behind by the length of the sleep. The reference point is now the moment the reply arrived. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
Timers fire at once when given a delay above 2^31 - 1 ms (about 24.8 days), so a huge reconcile interval, publish receipt timeout, ping timeout or connection timeout fired immediately; the connection timeout then aborted every attempt. Such values now count as the longest supported delay. A connection timeout that isn't a positive finite number now disables it, as for the other timeouts, and the subscription retry timer no longer keeps a Node.js process alive. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
Subscribing to a channel again while its unsubscribe request was still pending did nothing, so the subscription was lost once the unsubscribe was answered, until the next periodic reconcile. The client now cancels the pending unsubscribe and subscribes again, and Chelonia's reconcile treats such channels as not subscribed. This is an older bug, found while reviewing the pubsub recovery work. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
The docs said that no request is ever dropped when the client replaces a connection. Only subscriptions (with their KV filters) are sent again; pending unsubscribes are settled by the close, and replacements in a row back off. Also corrects what reconnectOnTimeout does, which connection options Chelonia sets itself, a broken link, and when the KV conflict sync and the side-effect safety notes apply. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
A broken reconnection made the pubsub tests wait forever, so CI hung instead of reporting a failure; waiting for a reconnection now gives up after a few seconds. The Chelonia test files share their setup through the test harness, whose fake server now uses the client's protocol constants. The retry budget tests check every request type, the timer helpers have their own tests, a test that failed about once in twenty runs is fixed, and test titles no longer name labels from an external report. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
When the relay rejects a published event as conflicting, Chelonia syncs the contract and tries again. If that sync failed (for example because of a network error), the publish was rejected although attempts remained. The failure is now logged and the publish tried again, as chelonia/kv/set already does. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
A request that the relay doesn't answer within its retry budget makes the client replace the connection. With reconnectOnDisconnection: false, nothing replaced it, so one slow request closed the client's only connection and lost every other subscription. In that mode the client now keeps the connection and drops only that request, as it did before the recovery work. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
chelonia/configure copied the pubsub connection options through a JSON clone, which turns Infinity into null and drops functions. Restating the default maxRetries: Infinity therefore made the client stop reconnecting after one failed attempt. The options now reach the client as given, as hooks already did. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
No change in behaviour. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
The publishReceiptTimeout docs said that Chelonia syncs at once when the pubsub client isn't subscribed to the contract; it also waits while a subscription is pending. The debugging guide now says what a relay that never answers one request still costs: a new connection every 80 to 90 s with the defaults, each one re-syncing every contract. The pubsub passages of AGENTS.md become a summary that links to the docs, so the two can't drift apart. Also fixes an ambiguous sentence. 💘 Generated with Crush Assisted-by: Crush:claude-opus-5-5
Opus 5.5 review of latest changesCode ReviewBase: Context: okTurtles/libcheloniajs#125, "fix: recover lost pubsub subscriptions after sleep and dead sockets" (fixes #121, CL-1 to CL-10). This is the third round. The two reviews of 1. 🟡 An
|
After a device slept or the network dropped, the pubsub client could silently stop receiving events: it gave up on unanswered subscriptions, kept half-open sockets around, and Chelonia never noticed the gap.
The client now replaces the connection instead of dropping a wanted request, abandons dead sockets right away (ping/connection timeout, offline, wake-up from sleep) and exposes its connection state. Chelonia re-checks its subscriptions after every reconnection and periodically, syncs contracts whose published events don't come back, syncs on a publish or KV write conflict, and keeps the server time estimate correct across sleep. Includes an in-process fake pubsub server and relay for tests, plus docs for the new options, events and status.
Fixes #121. See okTurtles/group-income#3176.
Assisted-by: Crush:claude-opus-5-5