Skip to content

fix(falcor-react-redux): preserve query error handling with delayed disposal - #19

Draft
lmeyerov wants to merge 2 commits into
masterfrom
viz-mod/falcor-delay-dispose-operators
Draft

lmeyerov wants to merge 2 commits into
masterfrom
viz-mod/falcor-delay-dispose-operators

Conversation

@lmeyerov

@lmeyerov lmeyerov commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Changes

Use the public source.subscribe(...) in DelayDisposeOperator.call so the source's lifted map/catch operators execute. The previous _subscribe(...) shortcut bypasses the query error handler when delayed disposal is enabled. Update the tracked compiled file and its existing source map, and add six dependency-free Node test cases wired through this package's test script (also discoverable by the root lerna run test). No dependency, version or runtime-engine change.

Reproduction and validation

cd packages/falcor-react-redux
npm test
# Test command uses the existing peer/package dependencies and builtin node:test.
# Tested on Node24.18.0 and Node26.3.0; test harness requires node:test support.

Actual six-case results on both Node versions: before 4 PASS/2 FAIL; after 6 PASS/0 FAIL. The failing-before cases prove a read error escapes the query's catch/error memo and throws through a next-only component subscriber. Positive controls verify the unscheduled path, actual successful values, deferred upstream disposal with ignored late values, and synchronous completion without extra reads. The package npm script also actually executes all six cases.

Local fail-before/pass-after tests used the existing locked viz dependency cache (RxJS5 and Falcor helpers). Upstream source/compiled adapter match viz's installed2.11.0; repository metadata2.10.1 is unchanged. Focused current-head85dc579 CI clean-installs only existing package dependencies, rebuilds using declared Babel6 tools and actually executes all six cases on both runtimes with no skips: Node24, Node26. This proves the scoped package build/test, not the entire monorepo. Official pinned actions, contents-read permissions and a ten-minute job bound; no new dependency/version/runtime-engine policy or package publication. The tracked source map also matches the method edit.

Actual app boundary and remaining failure

Graphistry #3587 contains the held-GET/loopback-source-switch repro. With the current viz client, two losses of one GET exhaust its allowed retry; the uncaught RxJS error interrupts Socket.IO Manager's close dispatch before it schedules reconnect. Chromium's pause-on-uncaught diagnostic identifies SafeSubscriber.error, OuterSubscriber.notifyError, DelayDisposeSubscriber._error, the Falcor error chain, and Socket/Manager onclose on the same stack.

An isolated viz client build applying exactly this adapter edit has passing no-fault inspector/search controls. Corrected fault probes reconnect after both losses with fresh transport IDs/changed actual server peers, same document/socket/session/viz PID and no uncaught error. Functional recovery PASS: uncached admin57 search, a new degree histogram, ordered inspector close/reopen and a fresh admin73 search all work. Earlier absent-input/fresh-action FAIL was confounded by unordered toggle clicks and requiring the exhausted original read to succeed; raw evidence is retained and the fixture corrected. The corrected unmodified-runtime fault still fails second reconnect.

State criterion now passes with separate app fixes: The earlier visible-inspector/inactive-toggle failure is retained as a historical capture. E 99599d187a separately corrects bottom-panel toolbar selection and pending table extents; this library PR itself does not implement those fixes. Final combined isolated inspector/search pairs PASS both reconnects, actual fresh admin57/new degree histogram, first-click close, reopen/admin73 and coherent toolbar state, with same PID/document/session/socket and no uncaught error. Matching final shipping-client repeated-read control passes but its two-loss fault still fails; the library remains unconsumed. Shipping single-loss pagination and actual consumer unmount/superseded-search controls pass separately. Disposal-delay race, histogram persistence and GPU/persisted/auth/real VPN/full master app baselines remain unresolved or UNKNOWN; no complete app acceptance. Final exact app captures/source hashes. Earlier retained proof: Corrected actual app captures/source hashes.

Native F21 app integration — round19

The same adapter byte edit at current library head85dc579 is now also tested in a separate native F Arrow21 client build against F runtime49778 (current manual/proof head7af5b6395c). Installed shipping2.11.0, package metadata2.10.1 and this PR's runtime source remain unchanged. One Vite transform uses public subscribe; no install/publication/adoption or additional consumer retry.

Five healthy native controls pass. Shipping inspector/search two-loss faults retain second-reconnect FAIL/pageerror Object; isolated candidate pairs PASS two newSIDs/changed actual loopback peers/same PID/document/session, callback-zero guard, actual freshadmin57/new degree histogram, first-click close/reopenadmin73/coherent toolbar, all native buffers77/1016/154/508 and no page errors. Twelve captures total: ten scoped passes/two shipping failures, including permanent entry and owner-observer verification. Exact source/build/raw traces; portable commands and limits. Earlier E Arrow3 app evidence above remains historical and separate.

The successful native fault observes three GET packets for the held paths over the whole workflow. Readonly ownership trace shows initial+one retry for one logical read, then a new logical read attempt1 (owners3/3/8, attempts1/2/1). Why the model starts that new operation is not established; this is not a universal two-request bound or proof of explicit close/reopen causality. Per-operation GETretry1/no SET/CALL replay is retained. New-read semantics remain a consumer follow-on question, with no blanket resubscription change here.

Manual native scenarios do not execute in NodeCI; this library's already-verified six-case Node24/26 clean-build CI remains current because library source/head is unchanged. Captured IPC/canned anonymous app metadata/pausedplay0 do not prove GPU filtering/layout, persistence/auth, private chats, OS VPN/tether or Kubernetes. Native backend restart histogram-loss FAIL remains separately in #3587. Candidate integration PASS is not shipping adoption or a release waiver; existing library/co-land policy below remains.

Policy and landing timing

Keep the single GET retry and never replay SET/CALL. No global error swallowing, automatic cache clearing, global disabling of delayed disposal, blanket resubscription or additional retry is introduced.

This is a separate follow-on fix attempt, linked from the current C+D+E landing #3581 and C/D/E/F review descriptions. It is not consumed or required to co-land by those current PR heads; publishing/consuming a library release and validating state/recovery require separate work before claiming repeated-loss acceptance. Existing CLI66/Helm102 still co-land with C+D+E before F. Issue/PR filing does not waive release regression criteria.

Draft, unassigned, no merge, package publication or deployment. No new infrastructure or MCP implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant