JavaScript SDK: add configurable request timeout (KSM-1209)

[stas-schaller]

Summary

JavaScript SDK: adds a bounded, configurable request timeout to all network calls (main API requests, file upload, file download, and the offline-cache fallback), closing a hang where a stalled or hostile server could block the caller indefinitely (CWE-400).

Changes

Fixed

• Both platforms enforce a fixed deadline built on AbortController, not Node's socket timeout option, which only resets on inactivity and can be held open indefinitely by a slow trickle of data. The deadline stays armed across the whole exchange, response body included, so a server that sends headers immediately and then stalls or trickles is bounded the same as one that never responds at all. Both platforms reject with a KeeperError naming the timeout that was actually applied, and a mid-body connection failure now rejects instead of leaving the caller waiting forever. No cryptographic material or transmission logic is affected. (KSM-1209)
• Because the deadline covers the whole body, the default also acts as a minimum-throughput requirement on a file transfer, not just a liveness check on an API call. Raise requestTimeoutMs, or the per-call timeoutMs, for a large download or upload.
• Timeout values are validated once, before any request is attempted: 0, negatives, fractional values below 1ms, NaN, and Infinity are rejected with a plain Error (a caller-input mistake, not a KeeperError) rather than silently collapsing to a near-instant timeout that fails every request. Values above setTimeout's 32-bit ceiling are clamped rather than truncated to 1ms.
• Defaults to 30 seconds; override via SecretManagerOptions.requestTimeoutMs, which now also reaches downloadFile, downloadThumbnail, uploadFile (each also gains its own additive, optional timeoutMs argument that wins over the configured default), and the cachingPostFunction / createCachingFunction offline-cache helpers.
• downloadFile and downloadThumbnail keep timeoutMs as their second argument and gain the options object as an additive third argument, so an explicit timeoutMs still wins over it.
• Node's response buffering re-copied the whole accumulated buffer on every network chunk (O(n^2) in body size), which could turn a large-but-healthy download into a spurious timeout purely from its own buffering cost. Chunks are now concatenated once when the response ends. (KSM-1342)
• armRequest now wires both the request's own error event and the abort signal itself, not just the former: Node only destroys a ClientRequest and emits error on it for an abort once a socket has been assigned, so a request still waiting on a stalled proxy CONNECT or a saturated agent pool previously never settled at all, regardless of the configured deadline. The two listeners are guarded against double-settlement so the common, socket-assigned case is unaffected.
• The offline-cache fallback (cachingPostFunction / createCachingFunction) no longer treats a deliberate client-side timeout the same as a real network failure: a timeout now propagates to the caller instead of returning a synthetic success built from stale cache. Both now validate timeoutMs before attempting a request rather than inside the same try/catch as the request, so an invalid value is rejected outright instead of being caught and mistaken for a transport failure. allowUnverifiedCertificate is forwarded through that same cache path for consistency with the direct request path.
• A cache-write failure on either platform (disk full, IndexedDB quota, private browsing) no longer discards or misrepresents an already-successful fresh response; only the next call's offline fallback is affected.
• uploadFile now validates its timeout before allocating an upload placeholder on the backend, instead of after, so an invalid value can no longer leave a file record pointing at content that was never uploaded.
• getSecrets no longer persists a caller-supplied serverPublicKey/serverPublicKeyId to storage before validating requestTimeoutMs; an invalid value now produces no side effects at all.
• fileUpload never reads the response body; it's now drained on both platforms (res.resume() on Node, res.body.cancel() in the browser) instead of left unconsumed, which previously kept the socket (and the event loop) alive after a successful upload on Node.
• The timeout error message no longer includes the request URL's query string, since file download, thumbnail, and upload URLs from the storage backend carry a time-limited access token there.
• DEFAULT_REQUEST_TIMEOUT_MS is exported from both the Node and browser entry points. On a runtime with no AbortController, the SDK keeps working with no timeout enforced rather than failing every request.
• The custom-caching-function-support example now carries the same deliberate-timeout-vs-transport-failure distinction as the real implementation it demonstrates.
• The CHANGELOG's retry-sleep note previously said the sleep between throttle retries is unbounded; it is capped at 176s plus up to 25% jitter (KSM-1035), so one postQuery call under sustained throttling is bounded, not unbounded. Corrected the note and the stale comment above downloadFile describing an argument position that never existed in a published version.

Maintenance

• Consolidated duplicate timeout-propagation test coverage: test/keeper.test.ts and test/timeout.test.ts had grown near-identical describe blocks; test/timeout.test.ts is now the sole home for this coverage. Restored as a describe.each over downloadFile/downloadThumbnail, so both functions get the same 5-case precedence/clamping coverage instead of one sharing an unobservable case.
• De-flaked a cache-fallback test that depended on the working directory not already containing a stray cache file.
• Added coverage for the AbortController-unavailable fallback path, previously untested, and fixed that same test to fail cleanly instead of crashing the Jest worker if the guard it checks is ever removed.
• Added a regression test for armRequest's socketless-abort path (drives the mock without simulating a socket assignment), a scaling assertion for the response-buffering fix (spies on Buffer.concat's call count rather than timing, so it can't flake), an empty-body case pinning data to null rather than a zero-length Buffer, and a forwarding test for postFunction (the default query path, previously uncovered).

Testing

cd sdk/javascript/packages/core
npm test

Full suite 228/228 passing; tsc --noEmit clean. Every new/changed test in this PR was verified to fail against the pre-fix code before the fix landed. Notable coverage: test/deadline.test.ts (timeout resolution/validation/clamping, AbortController-unavailable fallback), test/timeout.test.ts (requestTimeoutMs propagation from SecretManagerOptions through every network call site, including postFunction/getSecrets/downloadFile/downloadThumbnail/uploadFile), test/nodePlatform.test.ts/test/browserPlatform.test.ts (deadline enforcement, mid-body failures, fileUpload response draining, socketless-abort settlement, response-buffering scaling), test/cachingFunctions.test.ts (timeout-vs-network-failure distinction and cache-write-failure isolation in the offline-cache fallback).

Breaking Changes

None. timeoutMs/requestTimeoutMs are optional and default to 30s; existing calls that omit them behave the same except for gaining the bound (and, for a large file transfer, a throughput floor at the default - see the Fixed section above).

Related Issues

• Jira: KSM-1209, KSM-1342
• Merge order: rebase this branch last, after PR JavaScript SDK: fix config read errors silently degrading to an empty config (KSM-1266) #1132, PR JavaScript SDK: run throttle detection on the full response body, not the truncated slice (KSM-1251) #1144, PR JavaScript SDK: secure the caching fallback's transmission-key storage (KSM-1265) #1133, and PR JavaScript SDK: fall back and stop redundant writes for the pinned server public key, close a concurrent-write race (KSM-1256, KSM-1255) #1157. PR JavaScript SDK: secure the caching fallback's transmission-key storage (KSM-1265) #1133 removes cachingPostFunction (replaced by createCachingFunction); test/cachingFunctions.test.ts here will need retargeting onto createCachingFunction at that rebase, widening both of PR JavaScript SDK: secure the caching fallback's transmission-key storage (KSM-1265) #1133's caching closures to accept allowUnverifiedCertificate/timeoutMs.
• Follow-ups filed during review, not fixed here: KSM-1364 (examples//integration/ are outside this package's jest scope), KSM-1365 (deferred cleanup: duplicated request wiring in nodePlatform.ts, duplicated caching carve-out across platforms)

[mgallego-keeper]

Summary

Reviewed the request-timeout implementation in detail, including empirical reproduction of the Node vs. browser timeout semantics against live servers. The core mechanism (bounding a previously-unbounded wait) works, but the two platform implementations are not equivalent, and that gap undercuts this PR's own claims of parity ("Node ... rejects with a KeeperError; browser uses AbortSignal.timeout") and "Breaking Changes: None."

Correctness / security

1. Node's timeout is an idle timer, not a deadline (medium). nodePlatform.ts (lines 213, 235, 272) passes timeout to https.request, which resets on every byte of socket activity. browserPlatform.ts (lines 335, 360, 386) uses AbortSignal.timeout(), a fixed wall clock deadline. Reproduced with a server trickling 1 byte every 150ms for 3s at requestTimeoutMs=300: the Node request resolved successfully; the identical scenario under browser semantics rejected in ~300ms. A hostile server that stays just under the idle window can still hang a Node caller indefinitely, which is the exact scenario the changelog entry for this PR says it closes, on the SDK's primary server side target.

2. timeoutMs: 0 means opposite things on each platform, and nothing validates it (medium). timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS (platform.ts:1 defines the default) only substitutes on null/undefined, so an explicit 0 passes through unchanged. On Node this fully disables the timeout (reopens the DoS this PR fixes); on browser, AbortSignal.timeout(0) aborts nearly every request instantly. Both reproduced live. Related, lower severity: negative values throw a raw RangeError instead of KeeperError on both platforms; values above 2^32-1 throw on browser but only warn and clamp on Node. Recommend validating/clamping timeoutMs once, in keeper.ts, before it reaches either platform.

3. downloadFile/downloadThumbnail cannot inherit SecretManagerOptions.requestTimeoutMs (medium). uploadFile (keeper.ts:1408) forwards options.requestTimeoutMs to platform.fileUpload, but downloadFile/downloadThumbnail (keeper.ts:1398,1403) only accept an explicit timeoutMs argument, no options parameter. A caller who sets requestTimeoutMs once, expecting it to bound downloads too, silently gets the 30s default instead.

4. cachingPostFunction/createCachingFunction silently drop the new override (low). Both public exports (node/localConfigStorage.ts:54, browser/localConfigStorage.ts:151) declare only (url, transmissionKey, payload) and call platform.post with 3 args, so requestTimeoutMs never reaches platform.post for any consumer using the SDK's own offline cache helpers.

5. Browser timeout errors are a raw DOMException, never KeeperError (low/medium). Node wraps its timeout in new KeeperError(...); browserPlatform.ts's get/post have no try/catch at all, and fileUpload's catch re-throws unchanged. Code written against instanceof KeeperError (the pattern the CHANGELOG implies) will silently miss timeouts on browser.

Breaking change risk

6. AbortSignal.timeout() is called unconditionally in every browser network call, with no feature detection (medium/high). No typeof guard and no polyfill anywhere in the rollup browser build. On a runtime lacking this API (pre-2022 browsers, older embedded WebViews), every get/post/fileUpload call now fails immediately with a TypeError, a full break, not limited to calls that would have timed out. This contradicts "Breaking Changes: None": a runtime that previously worked fine with no timeout enforcement now hard fails on every call.

Test coverage

7. requestTimeoutMs propagation through the public API is completely untested (high). Zero occurrences of requestTimeoutMs/timeoutMs in keeper.test.ts or throttle.test.ts. Verified by mutation testing: dropping options.requestTimeoutMs from the call at keeper.ts:800 entirely still leaves the full suite at 73/73 passing.
8. downloadFile/downloadThumbnail/uploadFile are never invoked by any test.
9. No test proves a timeout rejection from the new code isn't retried forever by postQuery's retry loop (current behavior is correct, verified by repro, but it's unguarded and untested).
10. nodePlatform.test.ts's https mock never emits a response event; a coverage run confirms the success path lines added by this PR (fetchData and the three callback bodies) are never executed.
11. DEFAULT_REQUEST_TIMEOUT_MS's value is only checked against itself; mutating 30000 to 30 left the full suite green.

Process note

This PR (currently based on feature/KSM-1254-js-node-hash-tag) does not trigger test.js.yml, which only runs on PRs into master. The only passing checks are Socket Security scans; no test execution ran in CI. Ran the suite locally against this exact branch to confirm health: 8 suites, 73 tests, all passing.

Recommendation

Requesting changes on items 1, 2, 3, and 6: the Node/browser timeout semantics gap and the unvalidated 0 value both undercut the security rationale for this fix, and the browser hard dependency contradicts the stated "no breaking changes." Items 7-11 would meaningfully reduce the chance of a silent regression in this exact area and are worth adding here or in a fast follow-up.

[mgallego-keeper]

Summary

Re-reviewed the follow-up commit (36b73f12, "review fixes") against the items requested in the previous review. Most of the 11 original points are addressed, and two (items 5 and 6 below, browser error type and feature-detection fallback) are genuinely fixed with nothing further to flag. But the fixes for items 3 and 4 introduce new regressions, including two crash sites in shipped code and a security-relevant interaction with an existing, unresolved ticket. Requesting changes again.

Correctness / security

1. cachingPostFunction and createCachingFunction now turn a timeout into a fake success (high). Both (node/localConfigStorage.ts:73, browser/localConfigStorage.ts:220) still catch every exception unconditionally and return a synthetic {statusCode: 200, ...} built from the last cached response. Before this PR that only fired on a genuine network failure; now it also fires on the new, deterministic timeout (default 30s), so any updateSecret/createSecret/deleteSecret/uploadFile call that merely runs long silently reports success from stale bytes. This is the exact catch-all that KSM-1265 (an unresolved OWASP audit finding from the same audit epic as this PR's own KSM-1209) already flags as too broad; this PR makes it substantially easier to trigger rather than narrowing it the way KSM-1265's acceptance criteria require.

2. downloadFile is now called incorrectly at both of its only two real call sites (high). examples/javascript/hello-secret/hello.js:26 and ksm-azure-devops-secrets-task/index.ts:147 both call downloadFile(file, options), but the signature is (file, timeoutMs?, options?), so the options object binds to timeoutMs and every call throws Request timeout must be a finite number... got [object Object] before any network request. The previous commit had this correct (downloadFile(file, undefined, {storage})); this fix commit deleted the undefined placeholder. The Azure DevOps package also pins core ^16.6.3, so it fails to type-check against this signature today, separate from the runtime crash.

3. resolveTimeoutMs still produces a near-instant timeout for one input range (medium). deadline.ts:39 checks the raw value against <= 0 before flooring, so a value strictly between 0 and 1 (e.g. 0.5) passes validation, then floors to 0, reproducing the exact "every request fails in a couple of milliseconds" failure this validation was added to prevent. Not covered by test/deadline.test.ts.

4. allowUnverifiedCertificate now silently reaches platform.post from cachingPostFunction (medium). node/localConfigStorage.ts:62 forwards it where before it was always dropped (verification was always on regardless of caller config). This is a real TLS-verification behavior change for cached-mode consumers, not mentioned in the CHANGELOG.

5. downloadFile/downloadThumbnail's new options parameter silently ignores allowUnverifiedCertificate (medium). keeper.ts:1404 only reads options?.requestTimeoutMs; platform.get has no TLS-bypass parameter at all, unlike platform.post. A caller who passes the same options object they use elsewhere reasonably expects the same TLS behavior and doesn't get it, silently.

6. Node's armed deadline timer leaks on a synchronous request() throw (low/medium). nodePlatform.ts:217 (and the post/fileUpload equivalents) arm deadlineSignal() before calling request(), with no try/finally. A malformed URL or invalid header throws synchronously; the promise still rejects correctly, but clear() is never reachable, so the timer and its AbortController leak for the full deadline window. browserPlatform.ts avoids this with try/finally.

Comment accuracy

7. The comment justifying downloadFile's argument order cites the wrong ticket, inaccurately (medium). keeper.ts:1400 says timeoutMs "stays the 2nd argument (its original, pre-options position)" to avoid compounding "KSM-1265's already-shipped breaking change." KSM-1265 is not shipped (status: In Development) and is not a breaking change; it is the cache-integrity finding referenced in item 1 above. There is also no "original position" for timeoutMs on downloadFile, it took exactly one argument before this PR. Please also drop the ticket number from the comment regardless of the above; ticket references belong in the PR description and CHANGELOG, not source comments.

API surface / consistency

8. uploadFile has no per-call timeout override (low/medium). Unlike downloadFile/downloadThumbnail, keeper.ts:1418 always uses the global requestTimeoutMs, so a large upload that needs more than the 30s default has no escape short of loosening the timeout for every other call on that options object.

9. DEFAULT_REQUEST_TIMEOUT_MS is not exported from the browser entry point (low/medium). node/index.ts:9 does export * from '../platform'; browser/index.ts:9 uses a narrow named list that omits it. A browser/bundler consumer importing the new constant gets nothing. The new regression test imports via main, so this asymmetry isn't caught.

10. The documented custom-queryFunction example still drops the timeout silently (medium). examples/javascript/custom-caching-function-support/hello.js:18 forwards only 4 of the 5 arguments postQuery now passes. Anyone copying this canonical example gets no timeout enforcement, with no error, exactly the hazard items 1 and 4 above were supposed to close, just left open here.

Validation ordering

11. validateTimeoutMs runs after persistent side effects in postQuery (low). keeper.ts:801 validates inside the retry loop, after storage writes and payload encryption already happened. A bad requestTimeoutMs still mutates on-disk config before the validation error throws.

12. The timeout value is resolved twice, and custom queryFunctions get the unclamped raw value (low/medium). validateTimeoutMs (deadline.ts:82) calls resolveTimeoutMs only to throw, then returns the raw input; deadlineSignal resolves it again to actually clamp it. A caller with a custom queryFunction that does its own naive setTimeout gets the raw, unclamped value, including values above the 32-bit ceiling that MAX_REQUEST_TIMEOUT_MS exists to prevent, since that clamp only lives inside deadlineSignal.

Code quality

13. The abort/error-handler wiring is triplicated (low). nodePlatform.ts's get/post/fileUpload (and browserPlatform.ts's equivalents) each repeat the same block verbatim; a future fix has to be applied identically in six places across two files.

Recommendation

Requesting changes on items 1 through 4: the cache-masking regression, the two crash sites, and the fractional-timeout gap. These are either shipped-code-breaking or security-relevant. Items 5 through 13 would meaningfully reduce the chance of a follow-up incident and are worth closing out here rather than in a fast follow-up, given how many follow-ups this ticket has already needed.

[mgallego-keeper]

Summary

Follow-up to the two rounds of review already on this PR. The blocking items from the last round (36b73f12 to e1575ae5) are resolved and verified against a full local test run (181/181 passing). Two smaller things from that same pass are worth closing out before merge.

Findings

1. The new fileUpload response-drain fix has no test proving it (medium). nodePlatform.ts:333's res.resume() correctly fixes the event-loop-hang bug described in this commit's message (an unconsumed response body keeps the socket, and the event loop, alive after a successful upload). But the only related test change is adding a resume = jest.fn() stub to MockResponse in nodePlatform.test.ts, purely so the existing tests do not crash on the new call. Nothing asserts resume was actually invoked. Every other fix in this same commit got a precise behavioral test; this is the one exception. Suggest adding expect(mockResponse.resume).toHaveBeenCalled() to the existing "fileUpload resolves as soon as the response headers arrive" test.

2. resolveTimeoutMs's error message and doc comment no longer match its own boundary (low). The validation check moved from timeoutMs <= 0 to timeoutMs < 1 (deadline.ts:36), correctly closing the fractional-timeout gap from the previous review. But the thrown message ("Request timeout must be a finite number of milliseconds greater than 0") and the doc comment directly above resolveTimeoutMs ("must be a finite number above zero") still describe the old boundary. A caller passing 0.5 now sees "greater than 0, got 0.5", which reads as self-contradictory since 0.5 is in fact greater than 0. Worth updating both to state the real constraint (at least 1).

Recommendation

Neither item blocks merge on its own, and both are quick. Worth closing out here rather than in a follow-up, given how many rounds this ticket has already needed.

[mgallego-keeper]

Summary

Follow-up to the three rounds already on this PR. This pass empirically reproduced the core timing mechanism against live servers, ran mutation tests against the existing suite, and cross-checked two Jira tickets (KSM-1342, KSM-1343) filed during an earlier pass on this PR that never made it into this review thread. KSM-1343 (the Azure DevOps extension) is being handled separately via its own release and isn't covered below. The rest are new correctness findings plus real test-coverage gaps in what's already on this branch.

Correctness / security

1. (HIGH) Quadratic response-body accumulation turns this PR's own deadline into a spurious failure on large downloads (KSM-1342). src/node/nodePlatform.ts:214-218's fetchData still reallocates and copies the entire accumulated buffer on every data event (Buffer.concat([retVal.data, data])), O(n^2) in body size. Pre-existing on the release branch, but this PR is what turns it from "slow" into "fails": there was no deadline before, so a large download just took longer and still completed; now the deadline covers the whole response body, so the O(n^2) CPU cost alone can exceed it. Measured at the shipped 30s default: a 100MB body (delivered over loopback in ~100ms) times out at ~30.05s with KeeperError: ...timed out after 30000ms. Replacing the accumulation with an array of chunks concatenated once at res.on('end', ...) (preserving the existing null for-empty-body behavior at line 212, since downloadFile passes the value straight to platform.decrypt with no null check) resolves it cleanly: confirmed linear scaling afterward with the full suite still green. The fix is small, isolated to this one function, and untangled from the timeout plumbing itself; recommend folding it in here rather than shipping the regression for a fast-follow.

2. (HIGH) The new timeout/network-failure carve-out only recognizes KeeperError, so a plain validation error gets silently swallowed into a fake cache hit. node/localConfigStorage.ts:82 (cachingPostFunction) and browser/localConfigStorage.ts:224 (createCachingFunction)'s if (e instanceof KeeperError) throw e (added to fix round 2's "timeout served as fake success" bug) doesn't cover resolveTimeoutMs's validation errors, which deadline.ts:37 throws as a plain Error by explicit design (see the comment at deadline.ts:28-30: "matching this SDK's convention that KeeperError signals a failed interaction with Keeper's backend, not a caller-input/config mistake"). Calling either caching helper with an invalid timeoutMs (e.g. 0) falls through the instanceof KeeperError check, and with a cache present silently returns stale data as a 200; without one, throws the unrelated-sounding 'Cached value does not exist' instead of the real validation message. The guard's default is backwards for safety: it currently treats anything that isn't KeeperError as worth falling back on, when it should default to re-throwing and allowlist only the specific failure modes that actually deserve a cache fallback.

3. (browser-only) The same guard can now discard a good response. browser/localConfigStorage.ts:211-224: the guard wraps both the platform.post call and the subsequent storage.saveBytes('cache', ...) write (line 217). IndexedDB write failures (KSM-1332, already merged) are wrapped as KeeperError. So a fresh response can succeed, then the cache write fails (quota exceeded, private browsing, blocked upgrade); the guard re-throws immediately, discarding the already-obtained fresh secrets instead of returning them or falling back to stale cache. Node's equivalent doesn't have this problem, since a failed fs write there throws a plain Error. Worth special-casing "the fresh response itself already succeeded" so a same-call cache-write failure can't take down an otherwise-successful request.

4. uploadFile validates its timeout after a real network side effect. keeper.ts:1504-1508: prepareFileUploadPayload (1505) and postQuery(options, 'add_file', ...) (1506, which round-trips to the backend allocating an upload placeholder/URL) both run before validateTimeoutMs is ever called, inline at the platform.fileUpload call on line 1508. uploadFile(options, record, file, 0) with an otherwise-valid options.requestTimeoutMs throws only after that allocation, and the record/payload already carry a fileRef to content that was never uploaded. postQuery itself validates up front for exactly this reason (comment at keeper.ts:798-800); uploadFile's own override wasn't brought in line with that same invariant.

5. The shipped example still has the bug the SDK itself just fixed. examples/javascript/custom-caching-function-support/hello.js: cachingPostFunction (line 16) forwards timeoutMs into postFunction now, but its catch block (line 31) never got the instanceof KeeperError carve-out added to the real implementations; it unconditionally logs and falls through to the cache-read fallback (lines 32-48) regardless of error type. This file also doesn't import KeeperError at all today, so the fix needs that added too. Anyone copying this example, its entire stated purpose, gets the "deliberate timeout served as fake cache success" bug reintroduced in their own code.

Test coverage

6. Structural gap: examples/ and integration/ can never be exercised by this package's own suite, regardless of the diff. jest.config.js's roots is ["<rootDir>/test"], scoped inside sdk/javascript/packages/core; the example and integration directories are outside that package entirely. This is part of why round 2's two crash sites and item 5 above can all exist without any automated check ever running against them.
7. AbortController-missing fallback path has zero coverage. deadline.ts:57-59's typeof AbortController === 'undefined' branch: deleting that guard entirely (always constructing an AbortController) leaves the full suite at 228/228 green.
8. Two round-2 fixes are only partially tested. keeper.ts:801's validate-before-side-effect ordering is correct, but the existing test only asserts the query function was never called, not that the serverPublicKey/serverPublicKeyId storage writes (802-807) were actually skipped; the "no persistent side effect" half of that fix has no real coverage. Similarly, nothing asserts that postQuery's custom-queryFunction path receives the clamped (not raw) timeout for an over-MAX_REQUEST_TIMEOUT_MS input; only the direct downloadFile/downloadThumbnail paths are checked for that.
9. New test is real-filesystem-dependent and reproducibly flaky. test/cachingFunctions.test.ts:61-64 ("a non-timeout failure still falls back to cache") relies on cachingPostFunction's hardcoded relative 'cache.dat' path resolving to nothing, with no mock and no cleanup. Confirmed by planting a stray cache.dat in the working directory before running the suite: the test flips from an expected rejection to a resolved 200. The browser side of the same describe.each (line 26) is unaffected, since it uses inMemoryStorage({}); only the node case shares real, un-isolated state with anything else that runs from this directory, including item 5's example.

API surface / robustness

10. downloadFile/downloadThumbnail place options as the 3rd positional argument, after timeoutMs (keeper.ts:1494,1499), while uploadFile and everything else in this file places options first. The comment at keeper.ts:1485-1490 explains this was deliberate, to avoid stacking a second breaking change onto KSM-1265's in the same minor; that's a reasonable call, and worth keeping as-is for that reason. Flagging only because the inconsistency itself is what produced round 2's two crash sites, and remains a live footgun for any future caller who doesn't happen to read that specific comment.
11. armRequest's manual signal.addEventListener('abort', ...) (nodePlatform.ts:242) races Node's own internal signal handling on the same AbortSignal. Confirmed the SDK's handler wins today on Node 22, but that's current event-emission ordering, not a documented contract. browserPlatform.ts's pattern (checking signal.aborted synchronously inside the existing error handler, via asTimeout) is spec-guaranteed and would remove the race rather than relying on today's timing.
12. postQuery validates and resolves requestTimeoutMs once up front (keeper.ts:801), but each throttle/key-rotation retry gets its own fresh full timeout budget, and the sleep between retries is itself unbounded. Under sustained throttling a call can legitimately run for many minutes even with a small configured requestTimeoutMs. Not necessarily wrong, but worth a CHANGELOG line, since "bounded request timeout" currently reads as per-call rather than per-attempt.
13. browserPlatform.ts's fileUpload (line 444) never reads or cancels its response body, unlike the Node counterpart this same PR just fixed for exactly that reason (res.resume(), to stop the process staying alive). Lower severity here, since there's no demonstrated hang, but it's the same class of gap, left open on the other platform.
14. Minor duplication: the KeeperError carve-out and its comment are hand-copied into both platform files instead of factored into a shared helper (both already import from deadline.ts); item 3 above is a direct consequence of that duplication drifting once already. Separately, get/post/fileUpload (nodePlatform.ts) each still repeat an identical try/catch around synchronous request construction, in the same commit that extracted armRequest specifically to stop duplicating the adjacent abort/error wiring.

Recommendation

Requesting changes on items 1 through 5: item 1 is a direct, measured regression in this PR's own core mechanism; items 2, 3, and 5 all reintroduce some form of "the offline-cache fallback masks a failure as success," which is exactly what round 2 flagged and this round's fix only partially closed; item 4 risks a record left with a fileRef pointing at content that was never uploaded. Items 6 through 14 are worth closing out here given how many rounds this ticket has already needed, but don't need to block on their own.

[mgallego-keeper]

Round 5, on commit 64eae6ca.

CHANGES_REQUESTED. Round 4's fourteen items are now twelve fixed, one partly fixed and one accepted as is, which is solid work. I still cannot sign off, for four reasons, all narrow and all cheap. One round-4 fix backfired: removing armRequest's abort listener reintroduced an unbounded hang whenever the request has no socket yet, and I measured this head never settling where round 4's head rejected at the deadline. The 30 second default bounds the whole body, so a healthy 36 MB transfer that succeeded on the release branch now fails, and no note tells a caller. The merge with PR 1133 does not compile, and the resolution that silences the error silently drops the timeout. And round 4's highest-severity fix ships with no test, so the same defect can return with a green suite. I validated the armRequest fix and built the merge reconciliation, so two of the four come with tested answers rather than warnings. Two of the four problems trace to what this review asked for in round 4, and I say so in the body. Nothing here questions the design, which held up under every stall shape I could build.

First, a correction on what changed

The author said nothing changed and that the PR will probably need a rebase. Twelve commits landed after round 4 was posted, between 12:50 and 13:33 on 2026-09-03, and none of them had been reviewed. I read the remark as meaning no design change, and on that reading it is fair. The design is unchanged. But the code is not, and the twelve commits deserve a direct account.

Most of them are good, and several go past what was asked. fe9d2f3 reverts the Azure DevOps edit as out of scope, and I confirmed the revert is byte-exact against the base. d0ab07a hoists validateTimeoutMs out of the try on both platforms, and it isolated the cache write on the Node platform as well, which round 4 did not request. f049782 moves uploadFile's validation ahead of the add_file side effect and adds the example carve-out. 53ca672 covers the AbortController branch. a504942 de-flakes the cache test by mocking readFileSync, and I confirmed a planted stray cache file no longer changes the outcome. 93d674b closes two coverage gaps and finds a real write-ordering bug in fetchAndDecryptSecrets while doing it. 055ac35 drains the browser upload body. 44e0c5e and 64eae6c update the CHANGELOG. Twelve of round 4's fourteen items are genuinely closed, most with a test that fails when the fix is reverted.

Three commits also introduced new problems, and one of them is on us.

7c799dc did exactly what round 4 item 11 asked. Removing armRequest's abort listener removed the only unconditional settlement path, so a request that has no socket yet now settles nothing at all. Our recommendation was right about the ordering race and wrong to treat req.on('error') as a complete replacement. That is blocking item B1, and the review body should say plainly that the advice caused it. The same commit taught the https mock to emit 'error' on abort unconditionally and stated that as real Node behaviour, which is why the ten tests on that path cannot fail for the regression.

6803e65 and 04769e1 consolidated the timeout tests and dropped three downloadThumbnail cases that had been reviewed and signed off in earlier rounds. The consolidation itself is sound and two of its claimed strengthenings are real. The loss is narrow but it is a loss.

64eae6c added the CHANGELOG note round 4 asked for, and copied our wording, including the claim that the retry sleep is unbounded. It is capped at 220 seconds, and the same file says so two bullets above. That is our error reaching the release notes through the author's hand.

So the honest summary: the twelve commits closed round 4 well, and they opened one high-severity regression, one coverage loss, and one inaccurate release note. Two of those three trace back to what this review asked for.

Where this stands

Not yet, but it is close, and nothing outstanding questions the design. Four items need to change: restore a settlement path in armRequest, decide and document the file-transfer default, apply the tested merge reconciliation, and add one test for the buffering fix. Three of the four are small, and I have validated the fix for two of them. The reconciliation is built and green, so it is a checklist rather than an unknown. On current evidence I expect one more round, and a short one.

What this PR got right

The design is right and it is now well tested where it counts. The PR chose a fixed deadline built on AbortController rather than Node's socket timeout option, and that choice holds up empirically. I drove the built bundles against real local servers in every stall shape I could construct: no response at all, headers then stall, one byte every 500ms forever, a mid-body socket destroy, a stalled TLS handshake, and an upload into a server that reads slowly. Both platforms rejected within a few milliseconds of the configured value in every case, and the deadline stayed armed across the whole body, which is the property round 1 asked for.

The error discipline is good. A timeout is a KeeperError naming the value actually applied. A real transport failure passes through as itself, so connection refused still surfaces as a raw Error with ECONNREFUSED. The timeout message strips the query string, which matters because file URLs carry a time-limited signature. I checked all six paths and found no leak.

Validation is thorough. resolveTimeoutMs rejects zero, negatives, fractional values below one, NaN and both infinities, floors the rest, and clamps to the 32-bit ceiling. That closes round 1 item 2 and round 2 item 3, and each value is individually covered.

The response of these twelve commits to round 4 is the strongest part. Twelve of fourteen items are genuinely fixed, and several fixes go past what was asked. Commit d0ab07a isolated the cache write on the Node platform as well as the browser platform, which round 4 did not request. Commit 93d674b found and fixed a real write-ordering bug in fetchAndDecryptSecrets while closing a coverage gap. Commit fe9d2f3 correctly scoped the Azure DevOps integration out, and I confirmed the revert is byte-exact against the base. The O(n^2) buffering fix works: this head reads 200 MB over loopback in 394ms where the old shape needed 23.8 seconds for 64 MB.

The commit messages are unusually good. Each one names the round-4 item it answers, explains the reasoning, and reports the suite state. That made this review much faster.

Does the deadline actually work

The deadline works, and it works well, on every path except one. I drove the built bundles against real local servers rather than reading the code.

On the Node platform with the default agent, a silent TLS server that accepts and never answers rejected at 803ms for an 800ms deadline. A server that streams 36 MB steadily was cut off at exactly 30004ms on the 30000ms default, which proves the deadline stays armed across the whole response body and is not an idle timer. Round 1 item 1 is genuinely closed. A real transport failure still surfaces as itself: connection refused rejected at 7ms with a raw Error carrying ECONNREFUSED, not a KeeperError. The query string is stripped from every timeout message I saw.

The browser platform behaves the same way, and its fileUpload body drain uses res.body.cancel(), which is strictly stronger than the Node platform's res.resume().

The one real hole is socket assignment, and it is new since round 4. Node emits 'error' on a ClientRequest for an abort only after a socket exists. Commit 7c799dc removed the abort listener, so a request still waiting for a socket now settles nothing at all. Measured through setCustomProxyAgent with an agent that never yields a socket, this head printed NEVER SETTLED after 4006ms against an 800ms deadline. Round 4's head rejected at 802ms. The precise statement is that the deadline is unenforceable while req.socket is falsy. That covers a stalled proxy CONNECT and a saturated agent pool. It is blocking item B1, and I validated the two-branch fix: 801ms under the stuck agent, 804ms on the default agent, real errors unchanged.

Two smaller gaps, both non-blocking. Node fileUpload clears the deadline and then drains the response body, so a body that never ends holds the socket with no bound. I measured a process still alive at 6025ms after a 20000ms deadline had already been cleared. And the deadline is per attempt, not per call, which round 4 accepted; the CHANGELOG note describing that is factually wrong about the bound, which is a separate item.

Both platforms are covered. The empirical answer is yes, the timeout works against a real stalling server, with the one socketless exception named above.

Test coverage

I ran the baseline myself and then ran mutations. Head is 18 suites and 220 of 220 tests passing, with npx tsc --noEmit exit 0.

WHAT THE CONSOLIDATION DROPPED. Commits 6803e65 and 04769e1 deleted a 221-line describe('request timeout propagation') block from test/keeper.test.ts. Test-for-test, 12 of its 15 cases have real equivalents in test/timeout.test.ts. Three downloadThumbnail cases have none. I proved the loss by mutation: inverting downloadThumbnail's precedence at src/keeper.ts line 1506 leaves 220 of 220 passing with tsc clean, and the same mutation failed one test at round 4's head f1a454c. Two softer losses came with it. The uploadFile cases no longer exercise real key material, so a plausible key-id slip in prepareFileUploadPayload now survives. And no test pins uploadFile's own resolution of a valid options.requestTimeoutMs.

Two corrections to the concern as framed. The five-values-to-one trim on the invalid-value test.each was done by 04769e1, not 93d674b, which has 29 insertions and zero deletions. And that trim is safe: test/deadline.test.ts already sweeps 0, 0.5, -1, NaN, Infinity and -Infinity against resolveTimeoutMs, and each case is individually kill-capable. The deleted platform.cleanKeyCache afterEach is also not worth restoring, because its stated reason was the ownerKey and appKey the old cases seeded, and the new cases seed neither. So the instinct to audit hardest here paid off, but the part flagged first is the part that is genuinely fine.

WHICH FIXES ARE UNGUARDED. Four of round 4's five named fixes are covered, one test each. I confirmed the fifth myself: reverting fetchData to the release branch's per-chunk Buffer.concat leaves 220 of 220 passing with tsc clean. That was round 4's highest-severity item, and it is blocking item B4.

The untested set is larger than that one item. Mutations that survive with the suite fully green: postFunction's timeout forwarding, which is the default path for every consumer who supplies no queryFunction; armRequest's clear() on the request error event; getNotationResults' options forwarding; fetchData's if (chunks.length) empty-body guard, which round 4 explicitly asked to preserve; timer.unref, the browser cancel swallow, and the caching helpers' resolved-versus-raw forwarding. On the browser side, replacing the body read with an empty buffer, swallowing a mid-body rejection, moving clear() ahead of the body read, and giving the browser its own unredacted message builder all survive too.

One structural cause is worth naming. test/nodePlatform.test.ts's https mock emits 'error' on abort unconditionally, and its comment states that as real Node behaviour. So the 10 tests guarding armRequest assert the mock's premise, which is precisely why the B1 hang ships green. No test in this package starts a real server, so nothing exercises real Node abort semantics at all.

METHODOLOGY NOTE for any future mutation work here, which I hit myself. Ten test files import from '../', which resolves to dist/index.cjs.js. npm test rebuilds via pretest; npx jest does not. During the reconciliation I saw a mutation silently pass and six unrelated type errors appear, both purely from a stale bundle.

Reconciling with PR 1133 and PR 1157

I built the reconciliation and it is green. Numbers below are from a real merge in a throwaway clone, not a projection.

ORDER. Rebase this PR last, after PR 1132, PR 1144, PR 1133 and PR 1157. PR 1133 is the security fix and is further along, so making it redo this work would be wrong.

WHAT THE MERGE DOES. Starting at release tip f7c33bc and merging 7552c61, 68f066b, ed74e99 and 22d851f, the only conflicts are in sdk/javascript/packages/core/CHANGELOG.md. That four-PR tree is green: 18 suites, 198 of 198 tests, npx tsc --noEmit exit 0.

Merging this head then conflicts in exactly 5 files and 10 blocks:
examples/javascript/custom-caching-function-support/hello.js 1
sdk/javascript/packages/core/src/browser/localConfigStorage.ts 3
sdk/javascript/packages/core/src/keeper.ts 3
sdk/javascript/packages/core/src/node/localConfigStorage.ts 2
sdk/javascript/packages/core/test/keeper.test.ts 1
CHANGELOG.md auto-merges with no marker, and test/cachingFunctions.test.ts is staged as added with no marker.

STEP 1, the marked blocks. Take PR 1133's side whole for the three caching files, including examples/javascript/custom-caching-function-support/hello.js. Do not resolve that example toward this PR: git already took PR 1133's queryFunction call site and already deleted the fs require, so keeping this PR's block leaves createCachingFunction unimported and the example fails immediately with a ReferenceError on its main path. Union the import lists in src/keeper.ts and test/keeper.test.ts.

STEP 2, and this one bit me. In src/keeper.ts the ordering inside both conflicts matters. Put validateTimeoutMs FIRST, then persistServerPublicKeyOptions, and delete this PR's now-superseded saveString pairs, because PR 1157 replaced them with the helper. I unioned naively the first time, which put the storage write ahead of validation and silently reverted round 4 item 8. Exactly one test caught it, and only after a rollup rebuild. Note the asymmetry: getting postQuery wrong fails loudly with TS2304 on requestTimeoutMs, and getting fetchAndDecryptSecrets wrong compiles clean.

STEP 3, the nine hand edits git cannot help with. Widen both PR 1133 closures. Patch, verified to apply and compile:

--- a/sdk/javascript/packages/core/src/node/localConfigStorage.ts
+++ b/sdk/javascript/packages/core/src/node/localConfigStorage.ts
@@ -1,5 +1,6 @@
import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, platform, TransmissionKey, inMemoryStorage} from "../platform";
import {KeeperError, KeeperStorageError} from "../errors";
+import {validateTimeoutMs} from "../deadline";
import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes} from "../cache";
@@ -465,7 +466,7 @@
export const createCachingFunction = (
storage: KeyValueStorage,
options: {cachePath?: string, maxCacheAgeMs?: number} = {}
-): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean) => Promise => {
+): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean, timeoutMs?: number) => Promise => {
@@ -477,15 +478,23 @@

• return async (url, transmissionKey, payload, allowUnverifiedCertificate) => {

• return async (url, transmissionKey, payload, allowUnverifiedCertificate, timeoutMs) => {

•    // Resolved before the try below so an unusable value is reported as itself instead of
  

•    // being mistaken for a transport failure worth serving stale cache for.
  

•    const resolvedTimeoutMs = validateTimeoutMs(timeoutMs)
     let response: KeeperHttpResponse
     try {
         response = await platform.post(url, payload.payload, {
             PublicKeyId: transmissionKey.publicKeyId.toString(),
             TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey),
             Authorization: `Signature ${platform.bytesToBase64(payload.signature)}`
  

•        }, allowUnverifiedCertificate)
  

•        }, allowUnverifiedCertificate, resolvedTimeoutMs)
     } catch (e) {
  

•        // A deliberate client-side timeout is not a transport failure: serving stale cache
  

•        // here would turn a hung request into a fake success instead of surfacing it.
  

•        if (e instanceof KeeperError) {
  

•            throw e
  

•        }
         // A storage failure here (plausible during the same outage that took the network
  

--- a/sdk/javascript/packages/core/src/browser/localConfigStorage.ts
+++ b/sdk/javascript/packages/core/src/browser/localConfigStorage.ts
@@ -1,5 +1,6 @@
import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, TransmissionKey, platform} from "../platform";
import {KeeperError} from "../errors";
+import {validateTimeoutMs} from "../deadline";
import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes, concatBytes} from "../cache";
@@ -211,17 +212,23 @@
-export function createCachingFunction(storage: KeyValueStorage, maxCacheAgeMs: number = DEFAULT_MAX_CACHE_AGE_MS): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload) => Promise {
+export function createCachingFunction(storage: KeyValueStorage, maxCacheAgeMs: number = DEFAULT_MAX_CACHE_AGE_MS): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean, timeoutMs?: number) => Promise {

• return async (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload): Promise => {

• return async (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean, timeoutMs?: number): Promise => {

•    // Resolved before the try below, same reason as the Node helper.
  

•    const resolvedTimeoutMs = validateTimeoutMs(timeoutMs)
     let response: KeeperHttpResponse
     try {
         response = await platform.post(url, payload.payload, {
             PublicKeyId: transmissionKey.publicKeyId.toString(),
             TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey),
             Authorization: `Signature ${platform.bytesToBase64(payload.signature)}`
  

•        })
  

•        }, allowUnverifiedCertificate, resolvedTimeoutMs)
     } catch (e) {
  

•        // See the Node helper: a deliberate timeout must not become a fake cache success.
  

•        if (e instanceof KeeperError) {
  

•            throw e
  

•        }
         const appKey = await storage.getBytes(KEY_APP_KEY)
  

STEP 4, retarget test/cachingFunctions.test.ts. Do not delete it. Change line 3 to import createCachingFunction as createNodeCachingFunction, and change the describe.each entry to createNodeCachingFunction(inMemoryStorage({}), {cachePath: someTmpPath}). Two traps here. Pass an explicit cachePath, because PR 1133 defaults to a path under the user's home directory and a seeded app key would write there for real. And seed appKey with 32 bytes in the two cache-write-failure cases, because PR 1133 skips the write entirely without one, so a plain retarget leaves those cases passing while testing nothing. Assert the 'Failed to update cached response' log so the isolation is genuinely covered.

STEP 5, the two edits git will not flag. CHANGELOG.md auto-merges and its KSM-1209 entry still documents cachingPostFunction, an export deleted in the same release. Drop those mentions and describe createCachingFunction only.

RESULT, measured. After steps 1 to 5: npx tsc --noEmit exit 0, and 21 suites with 295 of 295 tests passing. A forwarding probe on the reconciled tree printed 'node: argc=5 arg4=true arg5=7777' and 'browser: argc=5 arg4=true arg5=7777', so the timeout genuinely reaches platform.post on both platforms.

VERIFY WITH npm test, NOT npx jest. Ten test files import from '../', which resolves to dist/index.cjs.js. npm test runs the rollup build in pretest. Without a rebuild I saw six misleading TS2307 and TS2353 errors and a mutation that silently passed.

Checked against the backend contract

No change to the bytes on the wire, and I checked rather than assumed.

CALL SITES. Only six network call sites exist in this package, and all six are now bounded: postQuery through postFunction to platform.post, which covers all eleven documented endpoints; postQuery through a custom options.queryFunction; the Node cachingPostFunction; the browser createCachingFunction; downloadFile and downloadThumbnail through platform.get; and uploadFile through platform.fileUpload. Every one receives a value that has passed validateTimeoutMs, so it is validated, floored and clamped. No unbounded network call remains in the package.

PAYLOADS. No field is added to any payload. The only keeper.ts type changes are SecretManagerOptions gaining requestTimeoutMs, which is a client-side options object that is never serialised, and the queryFunction function type gaining a fifth parameter. GetPayload, UpdatePayload and FileUploadPayload are untouched. clientVersion still uses the ms prefix. So there is no HTTP 400 risk from an unknown REQUEST field.

add_file. prepareFileUploadPayload is untouched. One reviewer captured the actual JSON handed to encryptAndSignPayload: the keys are clientVersion, clientId, fileRecordUid, fileRecordKey, fileRecordData, ownerRecordUid, ownerRecordData, ownerRecordRevision, linkKey and fileSize, in that order, with ownerRecordRevision matching the record's revision. Moving validateTimeoutMs to the first statement of uploadFile changed ordering relative to prepareFileUploadPayload, not the payload.

SIGNATURE AND TRANSMISSION KEY. generateTransmissionKey and encryptAndSignPayload are byte-identical to the base. The signature base is still the encrypted transmission key followed by the encrypted payload. postFunction appends timeoutMs after allowUnverifiedCertificate and sends the same three headers. In nodePlatform the only change to the request options is adding signal. Method, headers, Content-Length, the multipart boundary and the write order are unchanged. Validating before generateTransmissionKey is strictly better, because a bad value no longer burns a transmission key or writes config.

THE THIRD-PARTY UPLOAD. Confirmed empirically that the deadline covers the whole multipart body and not just the connect. Against a server that read the body slowly, fileUpload with an 800ms deadline aborted at 804ms after the server had received 304 bytes of 8 MiB. That is correct behaviour, and it is also the mechanism behind blocking item B3: one fixed 30 second default is a throughput requirement on an upload, not a liveness check.

REDACTION. All timeout messages come from one builder, timeoutError, which strips the query string. I confirmed all six paths, on both platforms, with a URL carrying a signature, and found no leak. Non-timeout URL-parse errors still carry the URL, which is pre-existing and is a nit above.

What is posted where

Four blocking items and 16 non-blocking items are inline on the lines they concern. 2 non-blocking items do not anchor to a line this PR changed, so they are below.

Non-blocking items with no anchor in this diff (2 items)

N4 (medium): The shipped caching example still discards a good response when its cache write fails, and lacks the validate-before-try ordering
examples/javascript/custom-caching-function-support/hello.js line 28

The KeeperError carve-out landed here, which closes round 4 item 5. The other two fixes the real helpers received in the same batch did not.

Line 28's fs.writeFileSync sits inside the outer try with no inner try/catch. So a cache-write failure lands in the catch, is not a KeeperError, and falls through to the stale-cache read. The example then returns statusCode 200 carrying older secrets while a fresh, successful response is thrown away. That is round 4 item 3's bug verbatim, in the file this example exists to be copied from.

Under identical conditions the SDK's own cachingPostFunction returns the fresh response. So the SDK already decided this behavior is worth guarding, in the same commit.

One correction to the obvious framing: on a genuinely full disk the outcome is different and arguably worse. fs.writeFileSync truncates before writing, so ENOSPC destroys cache.dat, and the example then returns a zero-length 200. The stale-secrets outcome comes from an unwritable path, such as a read-only file. Both are worth avoiding.

Please wrap the write at line 28 in its own try/catch with a short comment, mirroring src/node/localConfigStorage.ts. The validate-before-try half matters less, because postQuery validates options.requestTimeoutMs before any queryFunction runs, so only a direct caller of the example function can reach it.

Worth noting for scope: this file is pre-existing on the release branch, and no automated check can reach it, which is KSM-1364.

N15 (nit): The timeout carve-out keys on the whole KeeperError class, so callers cannot detect a timeout without matching message text
sdk/javascript/packages/core/src/errors.ts line 9

A design suggestion, not a defect. Today the carve-out is precise: platform.post can only produce a KeeperError from the deadline, on both platforms, and I confirmed that against real stalled, reset and refused connections.

The risk is that it keys on the base class. KeeperThrottleError and KeeperCryptoError already extend KeeperError. If a later change wraps a transport error in a KeeperError for a better message, the offline cache silently stops covering it. I tried that one-line change and both helpers stopped serving a valid, present cache for a TLS error and a socket hang up, with the suite still green at 220 of 220.

The caller-side half is a present-day gap. A timeout arrives as a bare KeeperError. A throttle exhaustion arrives as a KeeperThrottleError, which is also instanceof KeeperError. So the pattern your own test comment names as the contract matches both, and only message text separates them. That message embeds a URL, so a consumer regex is brittle.

Suggested fix, consistent with the precedent this SDK already sets: add KeeperTimeoutError extending KeeperError, return it from timeoutError in src/deadline.ts, re-export it beside the other error classes, and narrow the three carve-outs to it. Existing instanceof KeeperError handlers keep matching, so nothing breaks. The third site is examples/javascript/custom-caching-function-support/hello.js, so one change covers all of them.

Previously posted findings and their status at this head (6 items)

• PARTIALLY_FIXED (round round 4, item 2) Round 4 item 2: the cache-fallback guard's default is backwards, it should re-throw by default and allowlist only the failures that deserve a cache fallback
• STILL_OPEN (round round 4, item 6) Round 4 item 6: examples/ and integration/ cannot be reached by this package's jest suite
• MOOT (round round 4, item 10) Round 4 item 10: downloadFile and downloadThumbnail place options third, after timeoutMs
• STILL_OPEN (round round 4, item 14) Round 4 item 14: the KeeperError carve-out and the request try/catch are hand-copied instead of factored out
• STILL_OPEN (round round 2, item 5) Round 2 item 5: downloadFile and downloadThumbnail still do not honor allowUnverifiedCertificate
• PARTIALLY_FIXED (round round 2, item 1) Round 2 item 1: the caching helpers turn a timeout into a fake success from stale cache

Findings I checked this round and dropped, so nobody chases them again (7 items)

• The custom-caching example crashes inside its own catch block on the core version its package.json pins The premise is stale. The example does pin core 17.3.0 at this PR's head, and 17.3.0 has no exported KeeperError, so the instanceof line does throw a TypeError there. But the release branch tip f7c33bc ...
• fetchData's response-stream error path does not apply the timeout mapping that armRequest applies The asymmetry is real but the failure does not follow. Node attaches the abort signal to the ClientRequest itself, so an abort destroys the request first and the response stream's error is a strictly ...
• The CHANGELOG's claim of a bounded timeout on all network calls does not hold, because the cloud ... All three reviewers refuted this and I agree. The sentence scopes itself in the same breath: it reads all network calls followed by the parenthetical main API requests, file upload, file download. That ...
• On a runtime with no AbortController the timeout is silently not enforced at all The mechanism is real and I would have kept it, except the branch is unreachable on any supported runtime. package.json declares engines node >=20, and AbortController has been a Node global since Node ...
• A third-party Platform implementation silently keeps the unbounded hang, with no TypeScript error True as a type-system observation, but not a defect and not a regression. A function with fewer parameters is assignable to a type with more under every strictness flag, and no TypeScript construct can ...
• Three commit messages carry garbled test figures and cite a review round that does not exist The figures are not garbled. 232/228 and 221/236 are new total over previous total, and both denominators match the preceding commit's own measured total exactly. Two independent figures landing on the ...
• The offline-cache fallback's own security-relevant behaviour still kills no mutation Kept out of the review rather than refuted on the facts. Seven mutations do survive, including returning empty bytes instead of the cached bytes, dropping the 0600 mode, and caching a non-200 response. ...

Candidates for their own tickets (5 items)

• Cloud KeyValueStorage backends have no request deadline, so config load can hang at startup packages/aws, packages/azure, packages/gcp and packages/oracle each make their own network calls with no timeout and no abort signal. A grep for timeout, requestHandler and AbortSignal ... Existing ticket: none found
• Node offline cache accepts a truncated or zero-length cache file as a valid cache hit fs.openSync with mode 'w' truncates before writing, and fs.writeSync's return value is never checked, so both a failed write and a short write leave a partial file. The fallback's only ... Existing ticket: addressed by PR 1133 / KSM-1265
• KSM-1342 is still in Triage although its fix ships in this PR The quadratic buffering fix is in this PR, but the string KSM-1342 appears nowhere in the tree, in any of the 16 commit messages, or in the PR body's Related Issues list. The ticket is ... Existing ticket: KSM-1342
• examples/ and integration/ have no automated check of any kind Already filed, and confirmed still true. jest.config.js roots is test/ only, tsconfig.json includes only src/ and test/, and .github/workflows/test.js.yml filters on ... Existing ticket: KSM-1364
• Deduplicate the triplicated request try/catch and the copy-pasted caching carve-out Already filed and correctly deferred. Worth updating the ticket with one measurement: the duplication grew during this round rather than shrinking. The two caching helpers now share 38 ... Existing ticket: KSM-1365

Test status

Run by me at head 64eae6c in the read-only worktree, package sdk/javascript/packages/core, on Node v22.22.1.

npm test (runs npx rollup -c --bundleConfigAsCjs in pretest, then jest):
Test Suites: 18 passed, 18 total
Tests: 220 passed, 220 total
Snapshots: 0 total
Time: 4.578 s

npx tsc --noEmit: no output, exit code 0.

Working tree clean before and after (git status --porcelain empty). All five review worktrees verified at 0 modified files at the end. All mutation and merge work was done in scratch copies outside the worktrees.

Related numbers I measured, for context. Four-PR tree (1132, 1144, 1133, 1157 on release tip f7c33bc): 18 suites, 198 of 198, tsc exit 0. Reconciled five-PR tree including this PR: 21 suites, 295 of 295, tsc exit 0. Merged tree before the reconciliation: exactly one error, test/cachingFunctions.test.ts(3,9): error TS2305 on cachingPostFunction, and ts-jest reports the suite as failed to run.

[mgallego-keeper]

B2 (high): The merge with PR 1133 does not compile, and the naive resolution silently drops the timeout and the timeout carve-out

PR 1133 replaces cachingPostFunction with createCachingFunction. test/cachingFunctions.test.ts line 3 imports the removed export. That file is new on this side only, so git adds it with no conflict marker, and the tree then fails with error TS2305 on cachingPostFunction. ts-jest reports it too, so npm test fails, not only npx tsc --noEmit.

I ran the merge. With PR 1132, PR 1144, PR 1133 and PR 1157 already applied, merging this head conflicts in 5 files and 10 blocks: examples/javascript/custom-caching-function-support/hello.js 1, sdk/javascript/packages/core/src/browser/localConfigStorage.ts 3, sdk/javascript/packages/core/src/keeper.ts 3, sdk/javascript/packages/core/src/node/localConfigStorage.ts 2, sdk/javascript/packages/core/test/keeper.test.ts 1. CHANGELOG.md auto-merges with no marker.

Please retarget test/cachingFunctions.test.ts onto createCachingFunction. Do not delete it. Deleting it is the one resolution CI cannot catch. PR 1133's Node closure takes 4 parameters and its browser closure takes 3, while this PR's queryFunction type takes 5. TypeScript accepts the narrower function, so the timeout would simply stop reaching platform.post.

I proved that end to end. In the merged tree, a call through the unwidened closures reached platform.post with 4 arguments and an undefined fifth. After the reconciliation the same call reached it with 5 arguments and 7777 as the fifth, on both platforms.

Both closures need four edits each: add allowUnverifiedCertificate and timeoutMs to the returned closure, forward both to platform.post, call validateTimeoutMs before the try, and put the KeeperError re-throw first in the catch. Note that only PR 1133's Node factory takes an options object. The browser factory takes a positional maxCacheAgeMs number.

The full tested plan and the patch are in the reconciliation section. The reconciled five-PR tree is green at 295 of 295 with tsc clean.

[mgallego-keeper]

N14 (nit): Two residual cache-file issues in the Node helper, both pre-existing and both replaced by PR 1133

Recording these so they are not lost, not asking for a change here.

First, a failed cache write leaves a truncated file. fs.openSync with mode 'w' truncates before writing, and fs.writeSync's return value is not checked, so a short write also truncates silently. The fallback's only guard is if (!cachedData), and a zero-length Buffer is truthy, so a later call returns statusCode 200 with an empty body and a zero-length transmission key. Both halves are pre-existing on the release branch, and the new inner catch actually improves the same-call outcome, which previously discarded the fresh response too.

Second, both new inner catch blocks are bare, holding only a comment. So a container with a read-only working directory, or a browser hitting an IndexedDB quota, writes no cache and says nothing. Months later the network breaks and 'Cached value does not exist' is the first hint that offline fallback never worked. Returning the fresh response is right; only the silence needs fixing. PR 1133 already logs 'Failed to update cached response' in the same situation, which shows a log is appropriate here.

No change is needed on this PR, because PR 1133 replaces this whole function with writeFileAtomic and a size-bounded readCacheFile, which fixes both. If PR 1136 could ship first, add a console.error per catch and require cachedData.length > 32 next to the existing check.

[mgallego-keeper]

N16 (nit): Small accuracy items: an unsound generic, an empty error message, and two stale documents

Four one-line items, grouped.

validateTimeoutMs is declared as <T extends number | null | undefined>(timeoutMs: T): T and then casts. The body floors and clamps, so the declared type and the returned value can disagree. validateTimeoutMs(1500.9) has the static type 1500.9 and the runtime value 1500, and validateTimeoutMs(3000000000) has the static type 3000000000 and the runtime value 2147483647. No caller is affected today, and the helper is internal, so this is a nit. Overloads remove the cast and keep every call site compiling. The generic exists for a reason, so a plain number | undefined return would break src/keeper.ts line 1501.

The invalid-timeout message interpolates the raw value, so an empty string or an empty array produces a message ending in 'got ' with nothing after it. Prefer ${typeof timeoutMs} ${String(timeoutMs)} over JSON.stringify, because JSON.stringify renders NaN and Infinity as null, and those are the two most valuable diagnostics.

examples/javascript/file-upload/hello.js and its README both still state that uploadFile's response is never drained and leaves the process alive. res.resume() made that false. Please delete the two sentences. If the explicit exit stays, note that main().finally(() => process.exit(0)) also forces exit code 0 when main rejects, which hides the example's own error.

MAX_REQUEST_TIMEOUT_MS is the one new timeout constant no consumer can read. dist/deadline.d.ts declares it and there is no dist/deadline.js, so a deep import type-checks and then fails at runtime. src/platform.ts says the omission is deliberate, which is a fine answer; exporting it beside DEFAULT_REQUEST_TIMEOUT_MS would let test/timeout.test.ts import it from the public entry point like everything else in that file.

[mgallego-keeper]

N18 (nit): Non-timeout errors from the same calls still carry the query string, so a presigned-URL signature can reach a log

The redaction this PR adds is complete for what it claims. I confirmed all six timeout paths, on both platforms, strip the query string, and the CHANGELOG scopes the claim to the timeout error message, so it does not over-claim.

Non-timeout failures reject with the platform's own error, unchanged from the release branch. Of those, only a URL-parse failure carries the URL. Node's ERR_INVALID_URL puts it in an enumerable input property that JSON.stringify emits, and the browser's message embeds it. browserPlatform's console.error then writes it out. Connection, port and TLS errors do not carry the URL.

Reaching this with a live credential needs a malformed presigned URL from the backend, because file.url, file.thumbnailUrl and the add_file response.url all arrive verbatim from the server. So exposure is small and this is pre-existing.

One case is slightly more reachable and is browser-only: a well-formed URL carrying userinfo makes fetch reject up front with a message containing the password and the query token. If you want to harden this, route the catch blocks in the three Node and three browser calls through truncateUrlForError when the error carries the URL. Two scoping notes: truncateUrlForError keeps origin plus pathname, so a token in the path is still printed, and its split('?')[0] fallback keeps userinfo.

[mgallego-keeper]

Round 6, on commit 0a835ad34.

APPROVE, with one merge-order condition that belongs on whichever of PR 1133 or PR 1136 lands second. Nothing in this diff blocks. Three of the four round-5 blocking items are closed at 0a835ad, and two of them (B1 and B4) are now guarded by tests that I proved kill their own mutation. The fourth, B2, is a cross-PR sequencing item: PR 1136 merges cleanly against the release branch today, and the collision only appears after PR 1132 and PR 1133 land. I will not manufacture a blocker out of it. The remaining 12 items are low or nit, and 10 of them are unchanged carry-overs the author has already seen.

What the fix got right

B1 deserves explicit credit, because our own round-4 advice caused it. In round 4 we asked the author to remove the abort listener, on the grounds that it raced Node's internal handling of the same signal with no documented ordering guarantee. In round 5 we showed that removing it lost the only unconditional settlement path, so a request with no socket never settled at all. Those two asks pull in opposite directions, and the author found the synthesis that satisfies both: keep both listeners, and put one shared settled boolean in front of each, so the first settlement wins and the second is a no-op. The comment explains the mechanism correctly, and I checked it against the runtime rather than trusting it: ClientRequest.prototype.destroy ends in this.socket?.destroy(err), which is a no-op with no socket. The fix is also guarded now, which round 5 asked for: deleting the new listener fails exactly one test.

B4 is the other clean close, and it is closed in the strongest possible way. The author did not just add a test that passes. The test distinguishes the fixed shape from the reverted shape by the only observable difference there is, the Buffer.concat call count, because both shapes produce byte identical output. That is the right instinct on a performance fix.

Commit 15ba8c6 answers four separate posted items at once, not the two the mapping predicted: the B1 guard, B4, the empty body null contract from N9, and the worker crash from N17. Commit a1528cc restored the downloadThumbnail coverage as a describe.each over both functions with their own URL field, which is exactly the shape asked for, and the mutations that survived at the round-5 head now all die. Every added test is load-bearing; I found no filler.

Two more things worth saying. The author corrected both false claims in the downloadFile comment rather than defending them, and the replacement claim is accurate against the release branch and the published package. And the CHANGELOG split gave the buffering fix its own KSM-1342 bullet, and the PR body now names that ticket, which is the traceability we asked for.

Where this stands

Yes for this diff, on its own. The four new commits add one behavior change (the armRequest abort listener), and I could not break it in any direction I tried. The test suite grew from 220 to 228 and every added test is load-bearing. The one condition is not about this diff: whoever merges second, this PR or PR 1133, must apply the reconciliation, because the cheapest resolution compiles clean and silently reverts four separately reviewed fixes. That checklist needs one correction from us before it is used again: the STEP 3 patch we posted in round 5 does not apply with any tool.

Closure

Of the 4 blocking items, 3 are closed: B1 (fixed, and now guarded), B4 (fixed, proved by revert-and-run), and B3 (closed on the bar round 5 itself set, which was one plain migration sentence in the release notes). B2 is still open and is cross-PR only. Of the 18 non-blocking items, 4 are fully closed (N1, N2, N5, N17), 4 are partly closed (N3, N8, N9, N13), and 10 are still open (N4, N6, N7, N10, N11, N12, N14, N15, N16, N18). Nothing regressed. The four commits broke nothing that I could measure. New this round: 12 items, all low or nit, 3 of them created by the CHANGELOG rewrite in 0a835ad (a dropped contract statement, a double-counted retry budget, and one misattribution). Six round-5 claims of ours did not survive verification and are listed under refuted, including the open question F4.

B4: is the buffering fix guarded now

Yes, B4 is closed, and the revert-and-run proves it. I copied the package to a scratch directory, restored the release branch shape in fetchData verbatim (retVal.data = retVal.data ? Buffer.concat([retVal.data, data]) : data on the data event, with the single concat at the end event removed), then rebuilt and ran the suite.

Result: npx tsc --noEmit exited 0, npx rollup exited 0, and the suite went from 4 pre-existing scratch failures to 5. The single new failure is "request timeout > response handling > response chunks are concatenated once, not once per chunk", reporting Expected number of calls: 1, Received number of calls: 1999. So the reverted fix is caught by exactly one test and nothing else.

I checked the companion test the same way. Removing the if (chunks.length) guard, and nothing else, fails exactly one other test, "a response that ends with no data event leaves data null and never calls Buffer.concat". That preserves the round-4 point that a zero length buffer is truthy and would corrupt the error path in postQuery.

Both tests are therefore kill-capable, and the call-count assertion is the right instrument, since the two buffering shapes produce byte identical output and no assertion on the bytes could tell them apart. The 4 pre-existing failures in the scratch copy are the three proxy tests and the end to end secrets test, all fixture and environment dependent, and they are identical before and after each mutation.

B3: does documenting the floor close it

Documenting the floor closes B3, because it satisfies the bar round 5 itself set. B3's own wording was: either the default needs to differ for file transfers, or the release notes need one plain migration sentence, and shipping neither is the part I cannot sign off. The author took the second option. So B3 is closed, and only a nit remains.

The behaviour is unchanged, and this is the arithmetic. DEFAULT_REQUEST_TIMEOUT_MS is 30000 and the deadline covers the whole body, so a transfer must average size divided by 30 seconds for the whole exchange. That is 1.20 MB per second for a 36 MB file (9.6 Mbit per second), 3.33 MB per second for a 100 MB file (26.7 Mbit per second), and 34.13 MB per second for a 1 GB file (273.1 Mbit per second). Measured at this head against a real local server that answers headers at once and then streams at a fixed rate: a healthy 36 MiB body at 1 MiB per second, which needs about 36 seconds, completed in 36482 milliseconds on the release branch tip and rejected at 30004 milliseconds at this head, through downloadFile with no argument changes. The same shape at 800 KB and 100 KB per second rejected at 2004 milliseconds against a 2000 millisecond deadline and resolved at 8178 milliseconds against a 20000 millisecond one, so the deadline and not a stall is what decides.

What the note contains and what it does not. It contains the 30 second default in the sentence before the new one, so the threshold is present, and it names both override knobs. It does not contain a rate figure, the word floor that the commit title uses, or any statement that a call which previously succeeded now fails. It is also the only shipped prose: the package README is five lines and never mentions timeouts, so my round-5 README ask was not actionable and I withdraw it. Everything left here is the nit in item R6-10.

Merge order and the reconciliation

Re-measured today at the current heads, in a throwaway clone, with nothing pushed and no review worktree modified. Heads: PR 1132 at 587b32f, PR 1144 at 68f066b, PR 1133 at ae25255, PR 1157 at 90e896a, PR 1136 at 0a835ad, release branch tip still f7c33bc. PR 1132 and PR 1133 have both moved since some earlier measurements this round, and the result is unchanged.

Sequential merge in the order 1132, 1144, 1133, 1157. Step 1 fast-forwards. Steps 2, 3 and 4 conflict only in the package CHANGELOG.md, one block each.

Merging PR 1136 on top conflicts in exactly 5 files and 10 marker blocks: examples/javascript/custom-caching-function-support/hello.js with 1 block, sdk/javascript/packages/core/src/browser/localConfigStorage.ts with 3, sdk/javascript/packages/core/src/keeper.ts with 3, sdk/javascript/packages/core/src/node/localConfigStorage.ts with 2, and sdk/javascript/packages/core/test/keeper.test.ts with 1. Two files that matter get no marker: CHANGELOG.md merges automatically, and test/cachingFunctions.test.ts is staged as added.

After resolving all 10 blocks, npx tsc --noEmit printed exactly one error and nothing else: sdk/javascript/packages/core/test/cachingFunctions.test.ts line 3, TS2305, no exported member cachingPostFunction. Deleting that one file gave npx tsc --noEmit exit 0 and a green suite at 21 suites and 320 tests. On that green tree I probed the transport myself: platform.post received 4 arguments with the fifth undefined. PR 1133's Node closure at ae25255 takes 4 parameters, its browser closure takes 3, and its catch reads the app key and the cache with no KeeperError re-throw first, so the timeout carve-out is reverted too.

Both sides confirmed at the current heads. PR 1136 still imports cachingPostFunction at test/cachingFunctions.test.ts line 3 and still exports it at src/node/localConfigStorage.ts line 62. PR 1133 exports only localConfigStorage at line 188 and createCachingFunction at line 455 of the same file, and its browser factory is still at line 214.

The four new commits did not change the collision. Two of them touch conflicting files, src/keeper.ts and CHANGELOG.md, but the src/keeper.ts change is a comment rewrite about 500 lines from the nearest conflict region, so the conflict set and block counts are identical to round 5.

Corrections to our own round-5 material, all measured. The STEP 3 patch we posted does not apply with any tool: git apply reports a corrupt patch at line 8 and patch reports a malformed patch at the same line, because the hunk headers declare more lines than the abbreviated bodies supply. Treat both closure widenings as hand edits, and note that createCachingFunction has moved about 10 lines earlier, to line 455, with every context line still matching. The naive union in src/keeper.ts now fails 7 tests in 2 suites, 6 in test/keeper.test.ts and 1 in test/timeout.test.ts, rather than the 1 test round 5 reported. STEP 5 now has two CHANGELOG bullets to clean, not one. And the rollup build must run before npx tsc --noEmit, or ten module resolution errors bury the single real one.

GitHub reports all five PRs as mergeable and clean. For PR 1136 that is true against the release branch and says nothing about PR 1133, so no automated signal warns about this. The collision appears only after PR 1132 and PR 1133 land, which is when a rushed resolution is most likely.

What is posted where

13 items are inline on the lines they concern.

Previously posted items still open at this head (19 items)

• B2 STILL_OPEN, cross-PR sequencing only The merge with PR 1133 does not compile, and the naive resolution silently drops the timeout and the timeout carve-out
• N4 STILL_OPEN, untouched The shipped caching example still discards a good response when its cache write fails, and lacks the validate before try ordering
• N6 STILL_OPEN, untouched The browser fileUpload body drain can turn a successful upload into a TypeError
• N7 STILL_OPEN, untouched, and now correctly scoped as pre-existing Node fileUpload disarms the deadline and then drains the response body
• N10 STILL_OPEN, plus a third instance in fetchData's response error ... armRequest's clear() on the request error event, and the notation file read's options forwarding, are both unguarded
• N11 STILL_OPEN, untouched The uploadFile consolidation replaced real crypto coverage with stubs, and dropped two smaller assertions
• N12 STILL_OPEN, untouched The browser response body read, its deadline coverage, and its timeout message redaction are all unasserted
• N14 STILL_OPEN in the code, effectively moot Two residual cache file issues in the Node helper
• N15 STILL_OPEN, design suggestion The timeout carve-out keys on the whole KeeperError class
• N16 STILL_OPEN, all four nits An unsound generic on validateTimeoutMs, the raw value in the invalid timeout message, two stale example documents, and ...
• N18 STILL_OPEN, pre-existing nit Non timeout errors from the same calls still carry the query string
• N3 PARTLY CLOSED The retry sleep note said unbounded
• N8 PARTLY CLOSED The request mock encodes the assumption that fails
• N9 PARTLY CLOSED, effectively as intended Several guards in the new timeout code kill no mutation
• N13 PARTLY CLOSED The KSM-1209 CHANGELOG entry is one oversized bullet
• carry-over round 4 item 2 STILL_OPEN The cache fallback guard should re-throw by default and allowlist only the failures that deserve a fallback
• carry-over round 4 item 6 STILL_OPEN The examples and integration directories cannot be reached by this package's jest suite
• carry-over round 4 item 14 STILL_OPEN The request try and catch wiring and the carve-out are hand copied three times
• carry-over round 2 item 5 STILL_OPEN, documented downloadFile and downloadThumbnail still do not honor allowUnverifiedCertificate

Claims from my own round-5 review that did not survive verification (7 items)

I am recording these so they are not chased again, and because several were mine.

• The open question F4: a socket assigned after the deadline rejects may proceed on the wire, leak a socket, or ... Refuted unanimously by every angle that probed it, so this settles the question rather than leaving it open. Node's own signal handling marks the request destroyed at abort time, and the request then destroys ...
• The new double settlement guard has no observable effect, so its comment overstates what it does The guard is inert at the promise level, which is true and expected, but the comment does not overstate anything. Both listeners really do fire once a socket exists: measured, the abort listener settles and ...
• deadlineSignal unrefs the deadline timer, so the new socketless settlement path cannot fire in a process with ... The Node mechanism is real but there is nothing to fix and nothing new to say. The realistic socket starved case does fire: with an agent limited to one socket and a first request stalled in a TLS handshake, ...
• The PR 1157 conflict hazard is new this round The mechanism is real and reproduces, but it is not new. The posted round-5 review body already states it in the reconciliation section, including the exact asymmetry, that getting postQuery wrong fails loudly ...
• The three call sites that keep validateTimeoutMs but forward the raw value are a coverage gap with a real ... The mutants are equivalent, so no behavioural test can kill them and the stated failure cannot happen. At all three sites the resolved value has exactly one consumer, and that consumer resolves it again: every ...
• A stalled connection attempt is a resource leak this PR introduced Refuted as a defect in this PR. The leak count is identical on the release branch tip, at the round-5 head and at this head: three concurrent calls through a stalling proxy leave three sockets in all three ...
• The throughput note gives no threshold, is not in a place a caller reads, and a caller cannot express an ... Three sub-claims of the B3 residual, all refuted, which is why that item is now a nit. The 30 second default is stated in the sentence immediately before the new one in the same bullet, so the threshold is ...

Candidates for their own tickets (5 items)

• A stalled custom proxy agent keeps its pending connection open after the deadline rejects When a caller installs a proxy agent and that agent stalls inside its connect step, for example while waiting for a CONNECT response that never arrives, Node ... Existing ticket: none, needs a new ticket
• Compensating cleanup for a failed file upload leg add_file commits the file record and the owner record link before any content is transferred, and no code path removes either one when the upload leg then ... Existing ticket: none, needs a new ticket
• The examples and integration directories are unreachable by the package test suite The jest roots, the tsconfig includes and the CI path filter all stop at the core package, so no example is ever compiled or run. Round-5 item N4 lives ... Existing ticket: KSM-1364
• Factor out the duplicated request wiring and the timeout carve-out The same construction try and catch is hand copied three times in the Node platform, and the KeeperError carve-out is hand copied across both cache helpers. ... Existing ticket: KSM-1365
• The cache file permission gap and the inaccurate merged release note that claims it is fixed The Node cache write never re-asserts 0600 on an existing file, so a cache file created at 0644 by the shipped example, by a backup restore, or by the library ... Existing ticket: none for the note correction; the code fix rides on PR 1133

Test status

Measured by me at head 0a835ad in the read-only worktree, with the checked-in build already current (no source file was newer than the bundle, so no rebuild was needed and nothing was written to the worktree).

npx tsc --noEmit exited 0 with no output.
npx jest --maxWorkers=5: 18 suites passed of 18, 228 tests passed of 228, 1.595 seconds.

That is up from 220 tests at the round-5 head, and the rise of 8 reconciles exactly: 1 socketless armRequest case, 2 response buffering cases, 1 postFunction forwarding case, and a net 4 from replacing one downloadThumbnail case with a describe.each of 5 cases over both download functions.

Mutation checks I ran myself, each in a scratch copy of the package with a rollup rebuild before jest, and each restored afterwards. Reverting fetchData to the release branch per chunk shape: npx tsc --noEmit exit 0, and exactly one added failure, the concatenation call count test, reporting 1999 calls where 1 is expected. Removing the empty body guard: exactly one added failure, the null data test. The scratch copy has 4 pre-existing failures of its own, the three proxy tests and the end to end secrets test, which are fixture and environment dependent and identical before and after every mutation.

Cross-PR measurements ran in a throwaway clone, never in a review worktree and never against the base clone's checkout. The naively resolved five PR tree: one type error before the offending test file is deleted, npx tsc --noEmit exit 0 after, then 19 suites passed of 21 and 313 tests passed of 320, with the 7 failures being 6 of PR 1157's key persistence guards and 1 timeout validation test.

Housekeeping: both read-only worktrees report a clean git status, all scratch directories were removed, no npm install was ever run, and the orphan process check returned nothing.

[mgallego-keeper]

R6-1 (high (cross-PR sequencing, not a defect in this diff)): The reconciliation with PR 1133 is still not applied, and the cheapest resolution compiles clean while reverting four reviewed fixes

This is a merge-order item, not a change request on this diff. I re-ran the whole sequence at today's heads and it reproduces exactly as in round 5.

After PR 1132, PR 1144, PR 1133 and PR 1157 are merged, merging this PR conflicts in 5 files and 10 marker blocks: the caching example, the browser cache storage, the Node cache storage, src/keeper.ts and test/keeper.test.ts. Two files that matter do not get a marker. CHANGELOG.md auto-merges. test/cachingFunctions.test.ts is staged as added, and line 3 still imports cachingPostFunction, which PR 1133 no longer exports. After I resolved all 10 blocks, npx tsc --noEmit printed exactly one error, TS2305 on cachingPostFunction.

The risk is the one-line fix. I deleted that single test file. npx tsc --noEmit then exited 0 and the suite went green at 320 tests. On that green tree I probed the transport: platform.post received 4 arguments and the fifth was undefined, although src/keeper.ts passes the resolved timeout as the fifth argument. PR 1133's Node closure takes 4 parameters and its browser closure takes 3, and TypeScript accepts both for the 5 parameter queryFunction type. PR 1133's catch also has no KeeperError carve-out before it reads the cache, so a deliberate timeout goes back to returning a synthetic 200 from stale cache. Both CHANGELOG bullets still promise the opposite.

No commit in this round changes the collision. Two of the four commits do touch conflicting files, src/keeper.ts and CHANGELOG.md, but the src/keeper.ts change is a comment rewrite far from all three conflict regions, so the conflict set and the TS2305 outcome are identical to round 5.

Four corrections to the checklist I posted in round 5, all measured today.

1. Do not use the STEP 3 patch as a patch. It is an abbreviated diff whose hunk headers declare more lines than the bodies supply, so no tool applies it at any fuzz level. git apply reports a corrupt patch and patch reports a malformed patch. Treat both closure widenings as hand edits. As a hint, createCachingFunction now sits at line 455 in the Node cache storage file and its closure at line 470, about 10 lines earlier than the patch shows, and every context line still matches. The browser factory is still at line 214.
2. The naive union in src/keeper.ts is now much louder than round 5 reported. It fails 7 tests in 2 suites, not 1, because PR 1157 added write-once and torn-pair guards that this PR's duplicated saveString pairs break. STEP 2 is unchanged and still correct: validateTimeoutMs first, then persistServerPublicKeyOptions, and delete the superseded saveString pairs.
3. STEP 5 now has two bullets to clean, not one, because 0a835ad split the KSM-1209 entry and both halves still name cachingPostFunction.
4. Run the rollup build before npx tsc --noEmit when you validate the result. Ten test files type against the built bundle, so a stale build hides the real break behind unrelated module resolution errors.

One thing that has changed for the better: GitHub reports this PR as mergeable and clean, which is true against the release branch and says nothing about PR 1133. So no automated signal will warn about this. CI catches the compile error only if the resolver keeps the test file.

[mgallego-keeper]

R6-2 (low): One PR 1157 conflict hunk resolves silently, and a one word change would make it fail loudly instead

I am restating this rather than reporting it, because round 5 already published the asymmetry in the reconciliation section. It is still true, and one small change would remove the hazard for good.

The merge with PR 1157 conflicts twice in src/keeper.ts. In postQuery, the timeout is assigned and used later, so taking PR 1157's side alone fails the build with TS2304 on requestTimeoutMs. In fetchAndDecryptSecrets, the return value of validateTimeoutMs is discarded, so taking PR 1157's side alone builds clean, type checks clean, and drops the guard that an unusable requestTimeoutMs must produce no side effects. I built that resolution and confirmed it: rollup exit 0, npx tsc --noEmit exit 0, and one test fails, the first test in test/timeout.test.ts. That file is not in the conflict set, so the guard is not silent in CI. Neither pure side is safe either way: keeping this PR's side in postQuery reverts PR 1157's write-once persistence and fails 6 tests in test/keeper.test.ts.

The suggestion, which is new: give the fetchAndDecryptSecrets call a used result, for example assign it and pass it into postQuery. Any future resolution that deletes it would then fail to compile, exactly as the postQuery site already does.

[mgallego-keeper]

R6-3 (low): The CHANGELOG split dropped two consumer facing contract statements

The three way split is a real improvement, and the KSM-1342 bullet is exactly what I asked for. Two sentences left the file in the same edit and did not reappear.

First, the options third argument on downloadFile and downloadThumbnail. Deleting the old sentence was right, because its word keep claimed a published second position that never existed, which is what item N5 asked you to fix. The true half went with it. The surviving text says requestTimeoutMs also reaches downloadFile, downloadThumbnail and uploadFile, and the new throughput note tells the reader to raise requestTimeoutMs for a large download or upload. For uploadFile that works, because options is its first argument. For the two download functions it works only when the caller passes the options object as the third argument, since both read timeoutMs first and options?.requestTimeoutMs second. A caller who raises requestTimeoutMs on the object they hand to getSecrets and then calls downloadFile with one argument still gets the 30 second default. Please state the mechanism without the old compatibility claim.

Second, the queryFunction contract. SecretManagerOptions.queryFunction went from four parameters to five, and the shipped note no longer mentions it at all. TypeScript accepts a four parameter closure for a five parameter type, so an integrator with a custom transport keeps compiling and silently enforces no timeout. Please say that a custom queryFunction now receives the timeout as a fifth argument, floored and clamped when it is set, and left undefined when it is not, and that a callback which ignores it enforces nothing. The offline cache helpers receive the same value.

Both facts are covered by tests, so this is a release note gap only. A TypeScript caller can still discover the third parameter from the published types; a JavaScript caller, and anyone reading only the release note, cannot.

[mgallego-keeper]

R6-4 (low): The retry budget clause states a total as an increment, and the total it states belongs to a different scenario

The unbounded claim is gone and the constants are now correct. I checked them: at most 5 throttle retries, at most 3 key rotation retries, a 176 second cap on a server supplied retry_after, one sided jitter up to 25 percent, and no sleep on the key rotation branch. The replacement figure has three problems.

First, roughly 23 minutes is the total, not an amount on top of the per attempt timeout. The sleeps alone are at most 5 sleeps of 176 seconds plus 25 percent, so 1100 seconds, about 18 minutes and 20 seconds.

Second, the number does not match the scenario the sentence names. Sustained throttling alone gives 6 attempts, so about 21 minutes and 20 seconds at the 30 second default. The 23 minute figure needs the 9 attempt case, which is 3 key rotation retries followed by 5 throttle retries.

Third, the figure does not scale the way the wording implies. The real bound is 1100 seconds plus 9 times the configured per attempt timeout. At the 30 second default that is about 22 minutes and 50 seconds. At a requestTimeoutMs of 300000 it is about 63 minutes, where the sentence as written yields about 28 minutes. That matters, because the same bullet advises raising requestTimeoutMs for a large transfer.

Suggested wording: the sleeps add at most about 18 minutes, and the attempts add at most 9 times the configured per attempt timeout, which is roughly 23 minutes in total at the 30 second default and more if you raise it. Please also add the caveat I asked for in item N3: one public call can issue more than one postQuery, because getSecrets refetches once when the app was just bound.

[mgallego-keeper]

R6-5 (low): Both fileUpload drain issues are still open: the browser drain can fail a successful upload, and the Node drain runs with the deadline already cleared

Carry-over of items N6 and N7, both unaddressed and with no reply. The browser platform file is untouched by the four commits.

Browser half. Optional chaining guards only a null or absent body. A truthy body with no cancel method throws a synchronous TypeError inside the try, and the catch calls asTimeout, logs, and rethrows, so a successful upload becomes a failure. Reproduced end to end against a real local server: the server received the bytes and answered 201, and the call threw TypeError: _a.cancel is not a function. Note the message text: the build target downlevels the optional chain, so the diagnostic does not even name res.body. The environment that produces this is a consumer supplied fetch whose Response.body is a Node stream, for example node-fetch version 3, which supports native FormData yet exposes a Node Readable. Under node-fetch version 2 the upload itself is malformed, so there the effect is different but still bad: any server answer, including an error status, is replaced by a TypeError and the status code is lost. Real browsers and Node's own undici are safe, and with a real WHATWG response the cancel drain works correctly and releases the socket, so only the guard is missing. Please test the method rather than the object, or wrap the whole drain in its own try and catch, so no cleanup step can fail an upload the server already accepted. One test with a truthy body that has no cancel method closes it.

Node half, and I am correcting my own round-5 framing here. res.resume() is right and wanted: with a body that ends, the process now exits in about 48 milliseconds where the release branch needed about 6 seconds. The narrower point is that clear() runs before the drain, so the deadline never covers the drain, and res.resume() turns a passively held socket into an active unbounded download, measured at about 20 MB per second against a body that never ends. The release branch holds the same socket, so this is not a regression. Moving clear() to the response end and close events bounds it at the deadline, and I verified that variant works. No test pins the ordering today, so the line can be reverted with a green suite.

[mgallego-keeper]

R6-9 (low): A timeout on the upload leg leaves an allocated file record pointing at content that was never uploaded

One clause for the same bullet as the throughput note, and a separate ticket for the durable fix. No code change asked for here.

uploadFile calls postQuery with add_file first, which commits the file record and the owner record's new file reference, and only then calls platform.fileUpload. So a timeout on the transfer arrives after the allocation. I reproduced it against a real local server that consumes the body and never answers: the call rejected with the KeeperError, add_file had run once, and the owner record carried a file reference to content that was never stored. Retries accumulate: three attempts produced three add_file calls and three orphan file identifiers on the same owner record, and the backend does not stop that, because it skips the revision check for file uploads.

Two corrections to how I would have phrased this earlier. The caller is not completely without a handle: prepareFileUploadPayload appends the new identifier to the file reference field of the owner record object the caller passed in, and that mutation survives the throw. The thrown error itself carries no identifier. And the residue is not new. The release branch creates the same residue from a plain socket failure or a non 2xx status; it only differs in that the caller never learns and never retries. So this PR adds a more likely and repeatable way to reach a pre-existing problem.

Suggested clause: a timeout, or any failure, on the upload leg happens after the file record has already been linked to the owner record, so it can leave a file record with no content. Size the upload timeout generously and re-check the record after a failed upload.

[mgallego-keeper]

R6-10 (nit): The behaviour change is disclosed without a rate, and the option a caller reads has no doc comment

B3 is closed on the bar I set, so this is a nit and I am not asking for anything structural. Two small things would finish it.

The bullet gives the 30 second default in the sentence before the new one, so the threshold is present. What is missing is the plain migration phrasing and the arithmetic. At 30 seconds the transfer must average size divided by 30 seconds, so about 1.2 MB per second for a 36 MB file, about 3.3 MB per second for a 100 MB file, and about 34 MB per second for a 1 GB file. One added clause would carry it: a single download or upload that previously took longer than 30 seconds now fails with a KeeperError.

requestTimeoutMs in SecretManagerOptions has no doc comment. Two lines there would reach an editor tooltip and the published types, which is where a caller decides whether to set it. It must be a block doc comment, not a line comment: the declaration emitter strips line comments, which is why the comments on throttleSleep and onDecryptionError reach the published types bare. House style improvement rather than an inconsistency this PR introduced, since five of the eight fields carry no comment.

I withdraw two round-5 asks here. The README in this package is five lines with one heading, so a README timeout section is not actionable, and the release notes ship in the published package anyway. And exporting MAX_REQUEST_TIMEOUT_MS is a convenience, not a gap: I confirmed a caller can pass any large finite number and it is clamped rather than rejected.

[mgallego-keeper]

R6-11 (nit): The browser response body read, and its deadline coverage, still kill no mutation

Carry-over of item N12, unchanged. Replacing the browser body read with an empty buffer leaves the suite green, and against a real local server the call then returns zero bytes instead of the real body. Inserting clear() before that read also leaves the suite green, and against a server that sends headers and then stalls the call never settles; I measured it still pending at 10003 milliseconds against a 2000 millisecond deadline. The Node platform catches both classes with its three stalled body cases. Two browser tests close it: one response whose body read returns real bytes, and one stalled body case that asserts a KeeperError naming the deadline. The behaviour itself is correct at this head, measured at 811 milliseconds against an 800 millisecond deadline, so only the guard is missing. Also worth adding: test/deadline.test.ts still has no test for the exported timeoutError, which would cover the query string redaction on both platforms at once.

[mgallego-keeper]

R6-12 (nit): The KSM-1342 bullet attributes the quadratic buffering to the request timeout work

Small wording point in the new bullet. It opens with the Node platform re-copying, which is right, then calls the same cost the request timeout work's own buffering cost, which reads as if the timeout work introduced the copy. The per chunk copy is on the release branch already, and the new deadline is what makes its cost fatal. The source comment and the new test comment both get this right, and so does the PR body, so only this line needs the edit. Suggested wording: the copy was O(n^2) in body size, and the request timeout turned that pre-existing cost into a spurious timeout on a large but healthy download.

[mgallego-keeper]

R6-13 (nit): The new test comment says get() throws synchronously; the returned promise rejects instead

The mitigation is correct and needed. Only the stated mechanism is wrong. get() builds its promise with new Promise and calls deadlineSignal inside the executor, so a missing AbortController becomes a rejection, not a synchronous throw. The comment also contradicts its own code, which assigns the promise first and only then attaches the handler. I removed the guard to check: the call returned a promise, the promise rejected, and the suite reported a clean failure. Removing the handler as well killed the worker with a bare error and no summary, which is exactly what the comment says it prevents. Please change get() throws synchronously to the returned promise rejects. The commit message for this change is already accurate, so nothing else needs editing. One optional follow-on: the handler also hides the cause, since the reported failure then points at the mock helper rather than the missing guard. Recording the error and asserting it stayed undefined would name the real cause.