Repository navigation
fix(javascript): keep timer cancellation within Node's handle family - #7263
XiaoBinGan wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
XiaoBinGan
changed the title
 [Medium impact] The PR appears safe to merge; no blocking issues were found. SummaryTimer cancellation now links only schedules in the same Node handle family.
No actionable issues were found. Tests were not run during this review. Whether another PR supersedes this one could not be confirmed because GitHub access was unavailable. Reviews (5) · Last reviewed commit: "fix(javascript): keep timer cancellation..." · Reviewed by Greptile |
|
Note on the two failing checks: neither is caused by this diff.
The declaration in the description is complete ( |
b4bd1da to
6b74150
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b74150bf7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| timerHandleFamily(schedule.method) !== | ||
| timerHandleFamily(candidate.method) | ||
| ) |
There was a problem hiding this comment.
When a cancellation handle has one fully resolved cross-family origin (for example the added const h = setTimeout(...); clearImmediate(h) case), filtering the origin here makes resolveTimers set resolution: "unresolved"; JavaScriptSemanticGraphAsyncProjection.ts:209-215 then emits an ambiguous-target unknown. The target and incompatibility are actually known, so this reports false uncertainty (including the misleading “handle resolution was unresolved” detail) instead of a known no-op; model or suppress that state without generating an unknown.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
6b74150 to
cbdef36
Compare
|
The Verified on a clean checkout of and For this PR specifically: it adds one test file under
|
cbdef36 to
3237898
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Node keeps timeouts and immediates in disjoint handle families. clearTimeout and clearInterval are aliases that act only on a Timeout from setTimeout or setInterval, while clearImmediate acts only on an Immediate from setImmediate. Crossing the families returns without doing anything, so the scheduled callback still runs. REA linked a cancellation whenever the cleared binding traced back to any timer schedule, so clearImmediate(setTimeout(work, 10)) reported a complete cancellation and the graph emitted a resolved cancels-timer relation. It also flagged no uncertainty for it. Require the cancel and the schedule to share a handle family before linking them. A cross-family clear now keeps linkedTimerId null and resolution unresolved, and the graph reports the cancellation as a candidate with an ambiguous-target unknown instead of a resolved fact. Verified against Node v24.22.1 over all nine schedule/cancel pairs: setTimeout and setInterval with clearTimeout or clearInterval cancel, and setImmediate with clearImmediate cancels; the other five combinations leave the callback running. Node's own lib/timers.js shows why: clearTimeout requires timer?._onTimeout, clearImmediate requires immediate?._onImmediate and returns early otherwise, and clearInterval delegates to clearTimeout.
3237898 to
428548b
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Closing to free a review slot while This PR is source code, so its
Until those land, every source PR's The fix and its red/green evidence are unchanged and the branch stays rebased:
I will reopen it as soon as |
Summary
REA reports
clearImmediate(setTimeout(work, 10))as a completed cancellation and the semantic graph emits a resolvedcancels-timerrelation. In Node the timer keeps running: the fourclearTimeout/clearInterval-on-ImmediateandclearImmediate-on-Timeoutcombinations are silent no-ops. This makes timer cancellation respect Node's handle family.Problem and expected behavior
Node keeps timeouts and immediates in disjoint handle families.
clearTimeoutandclearIntervalare aliases that act only on aTimeoutproduced bysetTimeout/setInterval;clearImmediateacts only on anImmediateproduced bysetImmediate.Smallest trigger:
The callback still runs. Measured on Node v24.22.1, all nine pairs:
Node's own
lib/timers.json the same version shows why:clearTimeoutrequirestimer?._onTimeout,clearImmediaterequiresimmediate?._onImmediateand returns early otherwise, andclearIntervalis literallyclearTimeout.REA linked a cancellation whenever the cleared binding traced back to any timer schedule, without comparing the two methods. Before this change all nine pairs reported
resolution: "complete"with alinkedTimerId, andclearImmediate(setTimeoutHandle)produced acancels-timerrelation withresolution: "resolved"and no timer unknown.Expected: a cross-family clear establishes no cancellation link. Actual: it established a complete one and the graph asserted it as resolved.
Change and scope
resolveTimersnow requires the cancel and the schedule to share a handle family before linking them. A cross-family clear keepslinkedTimerId: nullandresolution: "unresolved", so the existing projection reports the relation as acandidateand raises anambiguous-targettimer unknown instead of a resolved fact.The family split is one small helper over the existing
TIMER_SCHEDULE_METHODS/TIMER_CANCEL_METHODSvocabulary; no new fields, relation kinds, or unknowns were added, and same-family behavior is unchanged.Intentionally not included: any change to
setTimeout/setIntervalmutual aliasing beyond treating them as one family, and any new resolution state for "known no-op" (an unresolved link with an explicit unknown keeps the gap visible without expanding the contract).Contract and boundary impact
src/domain/javascript/javascriptSemanticAsyncEffects.tsowns timer operations; the change is inresolveTimers, before graph projection. Consumers oftimerOperations(graph async projection, reconstruction obligations) see the correctedlinkedTimerId/resolutionpair and unchanged shapes.forksilent-stdio correction (fix(javascript): report fork's silent stdio as piped #7083).cancels-timerrelation can now be reported ascandidatewith anambiguous-targettimer unknown where it was previouslyresolved.resolution/unknown reporting only; observed facts are not removed, and a previously over-claimed relation becomes visibly uncertain.docs/public/product-catalog.json), package, or installation impact: none.Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.
(The analyzed fixture is parsed, never executed. The separate Node v24.22.1 probe that established the expected behavior ran
setTimeout/setImmediateinside the probe process only.)Evidence and regression coverage
keeps cancellation within the handle family that Node clearsinsrc/domain/javascript/javascriptSemanticAnalysis.timers.test.tsasserts all nine schedule/cancel pairs'linkedTimerIdandresolution.AssertionError: setTimeout then clearImmediate: expected { timerId: 'timer:cancel:50:72', …(9) } to match object { kind: 'cancel', …(3) }. With the change, all nine pairs match Node.clearImmediate(setTimeout(...)),clearTimeout(setTimeout(...))andclearInterval(setImmediate(...))yields 1 resolvedcancels-timerrelation plus twoambiguous-targettimer unknowns, where the same input previously yielded 3 resolved relations and no timer unknown.timers.htmland stable across supported versions.For boundary fixes: not applicable — no filesystem, process, or provider boundary is involved; this is a pure IR projection change covered by the domain tests above.
For evidence-bearing changes:
Validation performed
node --import tsxprobe over 9 schedule/cancel pairs, Node v24.22.1 — setTimeout/setInterval cancel with clearTimeout or clearInterval; setImmediate cancels with clearImmediate; the other five keep runningnode node_modules/vitest/vitest.mjs run src/domain/javascript/javascriptSemanticAnalysis.timers.test.ts— 7 passednode node_modules/vitest/vitest.mjs run --project domain— 8343 passed, 0 failednode node_modules/typescript/bin/tsc --noEmit -p .— exit 0npx oxfmt --check src/domain/javascript/javascriptSemanticAsyncEffects.ts src/domain/javascript/javascriptSemanticAnalysis.timers.test.ts— cleannode scripts/rea.mjs analyze-javascript-application <fixture> --format json— 1 resolvedcancels-timer, 2ambiguous-targettimer unknowns (previously 3 resolved, 0 unknowns)Compatibility, safety, and release
resolvedfor a cross-family clear is nowcandidatewith a timer unknown — the correction callers want.Author readiness
Human owner: @XiaoBinGan
Before merge
type(scope): outcome.