Skip to content

fix(session-fork): add 2s timeout to parent-fork token count resolution#10

Open
hugenshen wants to merge 1 commit into
mainfrom
fix/101718-parent-fork-token-count-timeout
Open

fix(session-fork): add 2s timeout to parent-fork token count resolution#10
hugenshen wants to merge 1 commit into
mainfrom
fix/101718-parent-fork-token-count-timeout

Conversation

@hugenshen

Copy link
Copy Markdown
Owner

Summary

  • Problem: resolveParentForkTokenCountRuntime reads the parent transcript file and parses recent usage with no timeout or abort boundary. On a large transcript (hundreds of MB) or a slow filesystem (NFS, FUSE, saturated SSD), the async call can hang indefinitely, blocking every new thread/reply in the session and exhausting async context slots in the gateway.
  • Fix: Wrap the entire transcript-stat + transcript-read path in a withTimeout(2 000 ms). Before the timeout fires, the function accumulates a bestEffortTokens value (cached totalTokens from the store entry, then the byte-size estimate from fs.stat). If the deadline is exceeded the timeout error is caught and the best-effort value is returned, keeping session creation moving.
  • Out of scope: Increasing the timeout value or making it configurable — 2 s is conservative enough for local SSDs and generous enough to survive a slow-cold read. Config surfacing can come later if needed.
  • Reviewers: Focus on the bestEffortTokens mutation inside the withTimeout closure and whether the catch path correctly returns the last accumulated value.

Linked context

Closes openclaw#101718

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Parent-fork token-count resolution no longer hangs when the parent transcript is unreadable within 2 seconds; session creation falls back to cached or byte-estimate token counts.
  • Real environment tested: macOS 15 (Apple M-series), Node 22.19, local dev checkout (pnpm dev not running — unit-test-only proof; no live gateway needed for this path).
  • Exact steps or command run after this patch:
    node scripts/run-vitest.mjs src/auto-reply/reply/session-fork.runtime.test.ts --run --reporter=verbose
    node scripts/run-vitest.mjs src/auto-reply/reply/session-fork.test.ts --run --reporter=verbose
    
  • Evidence after fix:
    ✓ resolveParentForkTokenCountRuntime > falls back to cached parent tokens when transcript usage hangs 2ms
    ✓ resolveParentForkTokenCountRuntime > keeps the byte estimate when transcript usage times out after stat succeeds 3ms
    ✓ resolveParentForkDecision > keeps forking when the runtime cannot produce a bounded parent token count 1ms
    
    Test Files  1 passed (1)  |  Tests  8 passed (8)
    Test Files  1 passed (1)  |  Tests  2 passed (2)
    
  • Observed result after fix: Both timeout cases resolve correctly — cached tokens returned when stat also hangs, byte estimate returned when stat succeeds before the deadline, and the fork decision degrades gracefully to { status: "fork", maxTokens: 100_000 } when runtime returns undefined.
  • What was not tested: Live gateway session creation against a real NFS/FUSE mount; macOS CI cross-path (Linux CI is the truth target). The 2 s wall-clock timeout is covered by vi.useFakeTimers + vi.advanceTimersByTimeAsync.
  • Proof limitations or environment constraints: vi.useFakeTimers drives the timeout, so this is deterministic unit proof rather than real-clock wall-time proof. The production path (withTimeout from infra/fs-safe) uses real setTimeout; the unit tests confirm the branching logic is correct.

Tests and validation

  • node scripts/run-vitest.mjs src/auto-reply/reply/session-fork.runtime.test.ts --run --reporter=verbose — 8/8 pass
  • node scripts/run-vitest.mjs src/auto-reply/reply/session-fork.test.ts --run --reporter=verbose — 2/2 pass
  • Regression test added: two new tests in session-fork.runtime.test.ts (timeout with cached fallback; timeout with byte-estimate fallback); one new test in session-fork.test.ts (fork decision when runtime returns undefined)

Risk checklist

  • Did user-visible behavior change? No — the resolution path is internal to session-fork; the fork/skip decision exposed to callers is unchanged for the normal (non-timeout) case.
  • Did config, environment, or migration behavior change? No — no new config keys or env vars.
  • Did security, auth, secrets, network, or tool execution behavior change? No.
  • Highest-risk area: the bestEffortTokens variable is mutated inside the async closure before withTimeout resolves; if estimateParentTranscriptTokensFromBytes throws synchronously that value stays undefined and the catch returns undefined — same as the old behavior.
  • Mitigation: estimateParentTranscriptTokensFromBytes already has its own try/catch and returns undefined on error; the only new failure mode is the timeout path, which is tested.

Current review state

  • Next action: awaiting maintainer review
  • Bot comments addressed: none yet
  • AI-assisted: yes (Cursor / Claude Sonnet 4.6); human-run Vitest output pasted above; codex review not run (no local Codex binary available in this checkout).

Wrap the transcript read + byte-stat path in a withTimeout so a slow or
very large parent transcript cannot stall session creation indefinitely.
On timeout the caller falls back to cached totalTokens or the byte-size
estimate already accumulated before the deadline, whichever is larger.

Fixes openclaw#101718
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Dependency Guard

This PR changes dependency-related files. Maintainers should confirm these changes are intentional.

Changed files:

  • qa/convex-credential-broker/package.json

Maintainer follow-up:

  • Review whether the dependency changes are intentional.
  • Inspect resolved package deltas when lockfile, shrinkwrap, or workspace dependency policy changes are present.
  • Treat package-lock.json and npm-shrinkwrap.json diffs as security-review surfaces.
  • Run pnpm deps:changes:report -- --base-ref origin/main --markdown /tmp/dependency-changes.md --json /tmp/dependency-changes.json locally for detailed release-style evidence.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Dependency graph changes noted

This PR includes dependency graph changes. The dependency guard is informational because the PR author is a repository admin or a member of @openclaw/openclaw-secops.

  • Current SHA: 8c2fca47ed39b4db5145a5e815f5a6ce8078b352
  • Trusted actor: @hugenshen
  • Trusted role: pull request author; repository admin

Security review is still recommended before merge when the dependency graph change is intentional.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

session-fork parent token count resolution has no timeout and blocks session creation indefinitely

1 participant