Skip to content

fix(nostr): clear per-relay publish timeout timer to prevent dangling handles#98720

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/nostr-profile-timer-cleanup
Jul 2, 2026
Merged

fix(nostr): clear per-relay publish timeout timer to prevent dangling handles#98720
vincentkoc merged 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/nostr-profile-timer-cleanup

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Closes #98463

publishProfileEvent in extensions/nostr/src/nostr-profile.ts creates a setTimeout per relay to enforce RELAY_PUBLISH_TIMEOUT_MS, but the returned timer handle was discarded. When a relay accepted the event quickly (the common success path), the timeout callback had already been scheduled and would remain active until it fired 5 seconds later. On busy agents that publish profiles repeatedly, these dangling timers accumulate and increase memory/FD pressure.

Why This Change Was Made

The fix saves the setTimeout handle and clears it in a finally block, so the timer is cancelled whether the publish succeeds, times out, or throws. This matches the timer lifecycle already used in sibling Nostr modules (nostr-bus.ts and nostr-profile-import.ts).

User Impact

  • Nostr profile publishing no longer leaks per-relay timer handles.
  • Long-running agents with frequent profile updates see reduced stale timer accumulation.
  • No behavioral change for callers: successes/failures/timeouts are still reported the same way.

Evidence

Unit tests in extensions/nostr/src/nostr-profile.test.ts:

✓ clears the per-relay timeout timer after a successful publish
✓ clears the per-relay timeout timer after a publish timeout
✓ does not add dangling timers when publishing to multiple relays

Run:

node scripts/run-vitest.mjs extensions/nostr/src/nostr-profile.test.ts --run

Result: 34 passed.

Lint: node scripts/run-oxlint.mjs extensions/nostr/src/nostr-profile.ts extensions/nostr/src/nostr-profile.test.ts → exit 0.
Typecheck: pnpm tsgo:extensions:test → exit 0.
Build: pnpm build → exit 0.

Real behavior proof

I wrote a standalone Node.js script that monkey-patches setTimeout/clearTimeout to count active "publish timeout" timers after the real exported publishProfile() function returns. The script runs against real Node timers (no Vitest fake timers).

BEFORE (source fix reverted):

--- success path (promise resolves fast) ---
relays: 2
successes: 2
failures: 0
active timers before call: 0
active timers after call: 2
dangling timer age: 2ms
dangling timer age: 1ms

--- timeout path (promise never resolves) ---
relays: 2
successes: 0
failures: 2
active timers before call: 0
active timers after call: 0

=== Summary ===
success path dangling timers: 2
timeout path dangling timers: 0
FAIL: dangling timers detected after publishProfile() returned

The leak appears only on the success path: when pool.publish resolves before the 5 s timeout, the old code never cleared the timer.

AFTER (this patch):

--- success path (promise resolves fast) ---
relays: 2
successes: 2
failures: 0
active timers before call: 0
active timers after call: 0

--- timeout path (promise never resolves) ---
relays: 2
successes: 0
failures: 2
active timers before call: 0
active timers after call: 0

=== Summary ===
success path dangling timers: 0
timeout path dangling timers: 0
PASS: no dangling timers detected after publishProfile() returned

Negative control: reverting only the source change while keeping the new tests makes the success-path test fail to observe a clean timer state, confirming the tests are sensitive to the fix.

  • Behavior addressed: per-relay setTimeout handle created during publishProfile is now cleared after the race finishes.
  • Real environment tested: Linux, Node v22.19+, local source checkout.
  • Exact steps or command run after this patch: node scripts/run-vitest.mjs extensions/nostr/src/nostr-profile.test.ts --run.
  • Evidence after fix: all 34 tests pass; standalone timer probe shows 0 dangling timers in both success and timeout paths.
  • Observed result after fix: no active RELAY_PUBLISH_TIMEOUT_MS timers remain after publishProfile returns.
  • What was not tested: live relay WebSocket behavior; the fix only touches local timer lifecycle and does not change relay protocol handling.

Related issues

@wangmiao0668000666

wangmiao0668000666 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@openclaw-barnacle openclaw-barnacle Bot added the channel: nostr Channel integration: nostr label Jul 1, 2026
@clawsweeper

clawsweeper Bot commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper

clawsweeper Bot commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 12:57 PM ET / 16:57 UTC.

Summary
The branch stores the Nostr profile publish timeout handle, clears it in a finally block, and adds publishProfile regression tests for success, timeout, and multiple-relay cases.

PR surface: Source +5, Tests +72. Total +77 across 2 files.

Reproducibility: yes. Current main creates a setTimeout inside publishProfileEvent without retaining or clearing the handle, and the PR body includes a standalone real-Node timer probe showing dangling timers before the patch and none after.

Review metrics: 1 noteworthy metric.

  • Publish timer regression coverage: 3 added. The new cases cover the success, timeout, and multiple-relay paths that define the timer cleanup invariant.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98463
Summary: The linked issue is the canonical bug report for the Nostr profile publish timeout leak; this PR and one other open PR are competing fix candidates, and an older similar PR is closed unmerged.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] A second open PR targets the same linked issue, so maintainers should choose one canonical landing path and close the other after merge.

Maintainer options:

  1. Decide the mitigation before merge
    Land one canonical Nostr profile timer cleanup that preserves this PR's regression coverage, then close the duplicate candidate and the linked issue through the merge.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • Human maintainer handling is needed only to choose between this tested PR and the other open candidate for the same issue; no automated repair is needed.

Security
Cleared: The diff only changes Nostr plugin timer handling and colocated tests; it does not touch secrets, workflows, lockfiles, dependencies, package scripts, or release surfaces.

Review details

Best possible solution:

Land one canonical Nostr profile timer cleanup that preserves this PR's regression coverage, then close the duplicate candidate and the linked issue through the merge.

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

Yes. Current main creates a setTimeout inside publishProfileEvent without retaining or clearing the handle, and the PR body includes a standalone real-Node timer probe showing dangling timers before the patch and none after.

Is this the best way to solve the issue?

Yes. Clearing the timeout owned by publishProfileEvent in a finally block is the narrowest maintainable fix; changing nostr-tools or moving the timeout to the bus caller would be broader than needed for this local timer lifecycle.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against db97bc1e4fcf.

Label changes

Label changes:

  • add P3: The PR fixes a narrow Nostr plugin resource-cleanup bug with limited blast radius and no reported outage, data loss, or security impact.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix copied terminal output from a standalone Node timer probe showing zero dangling publish timeout timers, plus focused unit-test results; no contributor action is needed for the proof gate.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix copied terminal output from a standalone Node timer probe showing zero dangling publish timeout timers, plus focused unit-test results; no contributor action is needed for the proof gate.

Label justifications:

  • P3: The PR fixes a narrow Nostr plugin resource-cleanup bug with limited blast radius and no reported outage, data loss, or security impact.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix copied terminal output from a standalone Node timer probe showing zero dangling publish timeout timers, plus focused unit-test results; no contributor action is needed for the proof gate.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix copied terminal output from a standalone Node timer probe showing zero dangling publish timeout timers, plus focused unit-test results; no contributor action is needed for the proof gate.
Evidence reviewed

PR surface:

Source +5, Tests +72. Total +77 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 6 1 +5
Tests 1 73 1 +72
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 79 2 +77

What I checked:

Likely related people:

  • vincentkoc: Current blame for publishProfileEvent points to 10d463d, and recent Nostr bus history includes multiple adjacent Nostr fixes by Vincent Koc. (role: recent area contributor and current line author; confidence: high; commits: 10d463d5ad8c, d4824f9a8f00, 1b20c1aca450; files: extensions/nostr/src/nostr-profile.ts, extensions/nostr/src/nostr-bus.ts)
  • steipete: The original Nostr channel plugin commit added the Nostr profile, bus, and channel surfaces that contain this publish path. (role: introduced behavior; confidence: high; commits: 7b6cbf58697c; files: extensions/nostr/src/nostr-profile.ts, extensions/nostr/src/nostr-bus.ts, extensions/nostr/src/channel.ts)
  • joelklabo: The original Nostr plugin commit credits joelklabo as co-author, making them plausible context for the initial profile publish design. (role: introduced behavior co-author; confidence: medium; commits: 7b6cbf58697c; files: extensions/nostr/src/nostr-profile.ts, extensions/nostr/src/nostr-bus.ts, extensions/nostr/src/channel.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jul 1, 2026
@harjothkhara harjothkhara mentioned this pull request Jul 1, 2026
Closed
@vincentkoc vincentkoc self-assigned this Jul 2, 2026
@vincentkoc
vincentkoc force-pushed the fix/nostr-profile-timer-cleanup branch from 6e58eee to c1d47eb Compare July 2, 2026 01:00
@vincentkoc

vincentkoc commented Jul 2, 2026

Copy link
Copy Markdown
Member

Land-ready at rebased head c1d47eb855c2b1890a76b0e02ccb9435eac63a6f.

  • Verified [email protected] returns one publish promise per normalized relay; OpenClaw owns the wrapper timeout and its cleanup.
  • The owner-local finally preserves existing success, failure, and timeout result mapping while clearing every per-relay timer.
  • node scripts/run-vitest.mjs extensions/nostr/src/nostr-profile.test.ts: 34/34 passed.
  • Real Node production-path probe: three wrapper timers scheduled for two successful relays and one timeout relay; zero remained active afterward.
  • Fresh branch autoreview: no findings, correctness confidence 0.98.
  • Exact-head hosted CI: all required checks complete with no failures.
  • Native scripts/pr review-validate-artifacts 98720 and OPENCLAW_TESTBOX=1 scripts/pr prepare-run 98720: passed.

This is the strongest canonical fix in the duplicate cluster; #98135 has the same source shape without equivalent regression coverage.

@vincentkoc
vincentkoc merged commit a7a444e into openclaw:main Jul 2, 2026
89 checks passed
@vincentkoc

vincentkoc commented Jul 2, 2026

Copy link
Copy Markdown
Member

Merged via squash.

vincentkoc added a commit that referenced this pull request Jul 2, 2026
@vincentkoc
Merge remote-tracking branch 'origin/main' into release/2026.7.1
28b8799 
* origin/main:
  fix(nostr): clear per-relay publish timeout timer to prevent dangling handles (#98720)
@Haderach-Ram Haderach-Ram mentioned this pull request Jul 2, 2026
Open
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 2, 2026
@wangmiao0668000666
fix(nostr): clear per-relay publish timeout timer to prevent dangling…
19aace3 
… handles (openclaw#98720)
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
@wangmiao0668000666 @chenyangjun-xy
fix(nostr): clear per-relay publish timeout timer to prevent dangling…
2d9b481 
… handles (openclaw#98720)
wheakerd pushed a commit to wheakerd/clawdbot that referenced this pull request Jul 15, 2026
@wangmiao0668000666 @RomneyDa
fix(nostr): clear per-relay publish timeout timer to prevent dangling…
f9fffe2 
… handles (openclaw#98720)

(cherry picked from commit a7a444e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

channel: nostr Channel integration: nostr P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

nostr: dangling timer in per-relay profile publish loop

2 participants

@wangmiao0668000666 @vincentkoc