Skip to content

fix(router): abort a joined preload when the router stops - #45

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
SebTardif:fix/joined-preload-abort
Oct 1, 2026
Merged

vincentkoc merged 1 commit into
openclaw:mainfrom
SebTardif:fix/joined-preload-abort

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Starting a navigation onto a preload that was still loading replaced the abort controller stored on that match. Stopping the router aborted the replacement. The loader kept the original signal. After the router started again, that late result could overwrite the new active match.

Why This Change Was Made

When navigation adopts a match that is still fetching, it keeps that match's abort controller. Stopping the router aborts the same signal the loader already holds. A preload that was not joined still stops the way it did before.

User Impact

Leaving a page while its preload is being adopted no longer lets the old loader replace the data for the next visit.

Evidence

Terminal output from the built router at /tmp/oc-pr-uirouter-f004, commit 8b4ec5b31ca262905f4d86a6b7edfd9a1c20eb4b:

joined_preload_signal_aborted=true
data_before_late_alice=bob
data_after_late_alice=bob
active_route=a
preload_flag=false

Command: node /tmp/oc-pr-uirouter-f004-proof.mjs

Real behavior proof

  • Behavior or issue addressed: A preload joined by navigation kept running after stop and could replace the next active match.
  • Real environment tested: macOS, Node, built package @openclaw/uirouter at commit 8b4ec5b31ca262905f4d86a6b7edfd9a1c20eb4b in /tmp/oc-pr-uirouter-f004.
  • Exact steps or command run after this patch: node /tmp/oc-pr-uirouter-f004-proof.mjs
  • Evidence after fix: terminal output from that command:
joined_preload_signal_aborted=true
data_before_late_alice=bob
data_after_late_alice=bob
active_route=a
preload_flag=false
  • Observed result after fix: Stop aborted the joined preload signal. The follow-up visit loaded bob, and resolving the old alice loader left the active data as bob.
  • What was not tested: browser history integration, and a joined preload whose loader redirects.

Validation

pnpm run check passed in /tmp/oc-pr-uirouter-f004 before the commit (format, typecheck, lint, 53 router checks, and pack).

Navigating onto an in-flight preload replaced its AbortController, so stop left the loader signal orphaned.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested a review from a team as a code owner October 1, 2026 01:50
@clawsweeper

clawsweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 1, 2026
@clawsweeper

clawsweeper Bot commented Oct 1, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 30, 2026, 9:53 PM ET / October 1, 2026, 01:53 UTC.

ClawSweeper review

What this changes

The PR makes navigation retain a joined preload’s abort controller, adds a stop-and-restart regression test, and records the fix in the changelog.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

✅ Ready for maintainer review

Current main still replaces the controller when navigation joins an in-flight preload. This PR addresses that cancellation gap, and its targeted regression test and built-package terminal trace support keeping it open for normal merge review.

Priority: P2
Reviewed head: 8b4ec5b31ca262905f4d86a6b7edfd9a1c20eb4b

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The narrow repair has targeted regression coverage and relevant built-package proof, with no blocking defect found.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR body provides a terminal trace from the built router at the pinned head showing that stop aborted the joined preload signal and that the old loader’s late result did not replace the new active data. The script itself was not supplied, so the trace is contributor-provided proof rather than an independently rerun observation. No stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides a terminal trace from the built router at the pinned head showing that stop aborted the joined preload signal and that the old loader’s late result did not replace the new active data. The script itself was not supplied, so the trace is contributor-provided proof rather than an independently rerun observation. No stored-data contract changes.
Evidence reviewed 6 items Current-main gap: At the pinned main revision, navigation always creates a new abort controller after cancelling the current run, including when it adopts an in-flight cached match.
Introduced repair: The introduced hunk retains a live controller from the active or cached match before creating a new one.
Cancellation boundary: Route loading suppresses publication when its original signal is aborted, and stop clears in-flight loading and route state.
Findings None None.
Security None None.

How this fits together

The router receives route catalogs, load contexts, and navigation or preload requests. It manages route matches and loader results, then exposes state for the application to display.

flowchart LR
  A[Preload request] --> B[In-flight route match]
  C[Navigation] --> D[Join existing load]
  B --> D
  D --> E[Shared abort signal]
  F[Router stop] --> E
  E --> G[Loader result handling]
  G --> H[Route state]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test delta production +10 net lines, tests +73 lines The small production change is paired with focused lifecycle coverage.

Technical review

Best possible solution:

Keep one live abort signal across preload adoption and navigation, with stale-result suppression verified across stop and restart.

Do we have a high-confidence way to reproduce the issue?

Yes. Current-main source shows the controller replacement, and the new focused test exercises preload adoption followed by stop and restart; this read-only review did not execute it.

Is this the best way to solve the issue?

Yes. Retaining the live preload controller at adoption directly repairs the ownership gap while preserving the existing stale-result guards.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 82b66b32de46.

Labels

Label changes:

  • add P2: This is a bounded router cancellation defect with plausible stale data after stop and restart, without evidence of a widespread urgent regression.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a terminal trace from the built router at the pinned head showing that stop aborted the joined preload signal and that the old loader’s late result did not replace the new active data. The script itself was not supplied, so the trace is contributor-provided proof rather than an independently rerun observation. No stored-data contract changes.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides a terminal trace from the built router at the pinned head showing that stop aborted the joined preload signal and that the old loader’s late result did not replace the new active data. The script itself was not supplied, so the trace is contributor-provided proof rather than an independently rerun observation. No stored-data contract changes.

Label justifications:

  • P2: This is a bounded router cancellation defect with plausible stale data after stop and restart, without evidence of a widespread urgent regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides a terminal trace from the built router at the pinned head showing that stop aborted the joined preload signal and that the old loader’s late result did not replace the new active data. The script itself was not supplied, so the trace is contributor-provided proof rather than an independently rerun observation. No stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a terminal trace from the built router at the pinned head showing that stop aborted the joined preload signal and that the old loader’s late result did not replace the new active data. The script itself was not supplied, so the trace is contributor-provided proof rather than an independently rerun observation. No stored-data contract changes.

Evidence

What I checked:

  • Current-main gap: At the pinned main revision, navigation always creates a new abort controller after cancelling the current run, including when it adopts an in-flight cached match. (src/router.ts:218, 82b66b32de46)
  • Introduced repair: The introduced hunk retains a live controller from the active or cached match before creating a new one. (src/router.ts:227, 8b4ec5b31ca2)
  • Cancellation boundary: Route loading suppresses publication when its original signal is aborted, and stop clears in-flight loading and route state. (src/loading.ts:215, 8b4ec5b31ca2)
  • Regression coverage: The new test checks that stop aborts the preload signal and that resolving the old loader after restart leaves the new active data intact. (test/router-joined-preload-stop.test.ts:52, 8b4ec5b31ca2)
  • Contributor runtime proof: The captured PR body reports a run against the built package at the pinned head: the joined signal was aborted, the active route remained a, and data remained bob after the old alice loader resolved. The proof script was not supplied for independent inspection. (8b4ec5b31ca2)
  • Feature history: Recent merged work on cancelled preloads and stale navigation appears in the router history; this supports routing to those area contributors without assigning fault. (src/router.ts, d9785c5b9122)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Sebastien Tardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Shakker: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@vincentkoc
vincentkoc merged commit a17a88d into openclaw:main Oct 1, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants