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)
• 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.
• armRequest's own abort listener raced Node's internal handling of the same signal, since request()/https.request() already destroys the request and emits error on it when the passed-in signal aborts. Removed the redundant listener; the existing error handler now checks signal.aborted to decide between the SDK's own timeout message and the raw error, mirroring the browser platform's existing pattern for the same problem.
• 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.

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.
• 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.

Testing

cd sdk/javascript/packages/core
npm test

Full suite 220/220 passing; tsc --noEmit clean. 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 getSecrets/downloadFile/downloadThumbnail/uploadFile), test/nodePlatform.test.ts/test/browserPlatform.test.ts (deadline enforcement, mid-body failures, fileUpload response draining), 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.

Related Issues

• Jira: KSM-1209
• 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.