Skip to content

fix(call-logs): avoid esbuild zombies in source workers - #18511

Merged
diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.53from
AiCaptain357:fix/call-log-artifact-zombie-cleanup
Nov 4, 2026
Merged

diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.53from
AiCaptain357:fix/call-log-artifact-zombie-cleanup

Conversation

@AiCaptain357

Copy link
Copy Markdown
Contributor

Source checkouts run the call-log artifact worker through TSX. On Linux, terminating that worker after its 30-second idle timeout can leave an esbuild zombie under the server process. Repeated cycles can exhaust a configured PID limit and prevent new workers or healthchecks from starting.

Run the source worker with Node's built-in TypeScript support and explicit relative imports. All supported Node versions already provide type stripping. Packaged JavaScript workers keep their existing priority; queue, idle timeout, drain and ref/unref behavior stay unchanged. The TSX cache workaround is no longer needed.

Validation:

  • The regression fails on the upstream baseline with three extra esbuild child processes, then passes with the fix.
  • Real artifact writes, idle restarts, forced close and CLI preload isolation are covered. The bundled worker is also exercised in standalone and scoped node_modules layouts.
  • Node 26.11.1: 64 focused tests across 14 files pass. Node 22.22.2 and 24.0.0: 10 source/packaging/lifecycle checks pass each.
  • Focused ESLint and formatting pass. Full unit/Vitest suites, the project typecheck and the application build were not run.

Related: #18056, which handles idle process exit.

⚠️ base-red inherited: #17219. The target release branch already has failing checks tracked there.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Call-log artifacts are now written more reliably, including when the app is running from a source checkout or a packaged installation.
    • Idle worker shutdown and forced termination no longer leave behind unnecessary background processes.
    • Call-log writes and shutdowns are better covered for slow startup and busy conditions.

Source call-log artifact workers now launch without the tsx/esm loader. Module imports use explicit .ts paths. Tests cover artifact output, worker lifecycle, preload behavior, esbuild child processes, and bundled worker resolution.

Changes: Call-Log Artifact Worker

Layer / File(s) Summary
Native TypeScript worker launch
src/lib/usage/callLogArtifactWriter.ts, src/lib/usage/callLogArtifacts.ts, src/lib/logEnv.ts, changelog.d/fixes/0000-call-log-worker-native-typescript.md
Source TypeScript workers now use empty execArgv instead of tsx/esm, and ensureWorker passes those arguments directly to Worker. Related imports use explicit .ts paths. The changelog records the stated effect on esbuild zombies.
Artifact and worker lifecycle tests
tests/unit/call-log-artifact-worker-native.test.ts, tests/unit/call-log-artifact-worker-tsx-cache.test.ts, tests/helpers/callLogSettle.ts, tests/unit/call-log-save-drain.test.ts
Tests cover artifact contents and metadata, idle restarts, forced close behavior, Linux esbuild child processes, and a subprocess preload case. The former TSX cache-option test is removed. Comments describe cold starts and hang guards.
Worker resolution and bundled execution
tests/unit/call-log-artifact-worker.test.ts
Resolution tests cover source and colocated JavaScript workers without loader arguments. Bundled-worker tests cover standalone and scoped node_modules layouts and verify artifact output and metadata.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: Minimal · up to bb412

Source workers use native TypeScript, while packaged builds retain JavaScript workers. No merge-blocking issue was identified.

Security architecture risk: Minimal · up to bb412

The runtime change removes a child-process leak without expanding access or permissions. Existing deployment selection, write ownership, and shutdown behavior remain unchanged.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The runtime change directly affects servers selecting a source TypeScript worker. Its sensitive outcome remains writing call-log data beneath the parent's resolved data directory, using existing server authority. Packaged JavaScript workers retain their previous launch path.

Security Findings and Attack Paths

  • inferred — No introduced or worsened attack path was identified in the inspected launch-to-write flow. Artifact contents remain data passed to the file writer rather than executable-module selectors. The routed subprocess creation and process inspection in the native test are test-only operations, not newly exposed production entrypoints.

Trust Boundaries and Controls

  • observed — The parent continues to select the worker executable and per-write environment. Empty execArgv avoids inheriting parent CLI preloads; a regression supplies a preload that throws outside the main thread and asserts a successful worker write. The worker remains an execution compartment, not a lower-privilege security sandbox.

Resilience and Maintainability Implications

  • observed — Worker failure settles outstanding jobs to null rather than leaving callers waiting. The persistence consumer publishes ready metadata only after a successful result; failure reaches the missing-detail path. Drain regressions assert an existing file for ready metadata and null metadata after forced close, preserving failure containment without making logging success a prerequisite for request processing.
Pre-merge checks: Passed 5 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description explains the operational problem, the implementation, related issue, validation results, and unrun checks. It does not follow the required template and omits or incompletely covers Tar… Rewrite the description using all template headings. Add the target branch checklist status, change type, focused tests and category gates, lint and reconciliation status, the complete list of changed test files, coverage details for the to…
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 5 passed
Check name Status Explanation
Title check Passed The title follows the required Conventional Commits format and accurately describes the main change: preventing esbuild zombie processes in source call-log workers.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Tests For Production Code Passed Production code changed under src/ in three files. The PR adds tests/unit/call-log-artifact-worker-native.test.ts and updates tests/unit/call-log-artifact-worker.test.ts, plus related call-log l…
No Ai Attribution Passed No forbidden AI attribution appears in the reviewed commits or PR diff. The three commit bodies have no trailers or generation footers, and the authored PR body contains neither a Co-Authored-By trail…
Full details: Tests For Production Code

Explanation

Production code changed under src/ in three files. The PR adds tests/unit/call-log-artifact-worker-native.test.ts and updates tests/unit/call-log-artifact-worker.test.ts, plus related call-log lifecycle test helpers. This satisfies the requirement to add or update tests for production changes.

Full details: No Ai Attribution

Explanation

No forbidden AI attribution appears in the reviewed commits or PR diff. The three commit bodies have no trailers or generation footers, and the authored PR body contains neither a Co-Authored-By trailer nor a “Generated with <AI tool>” footer. The commit author name does not create a prohibited trailer.

Full details: Description check

Explanation

The description explains the operational problem, the implementation, related issue, validation results, and unrun checks. It does not follow the required template and omits or incompletely covers Target Branch, Tests Added Or Updated, Coverage Notes, and Reviewer Notes. The validation checklist also does not identify the change type or confirm all required gates.

Resolution

Rewrite the description using all template headings. Add the target branch checklist status, change type, focused tests and category gates, lint and reconciliation status, the complete list of changed test files, coverage details for the touched src/ files, and reviewer notes for risks or manual validation. Preserve the existing summary, validation results, and related issue information.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 7 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

  • Mode: Continue automatically
  • Reviewed files: 8
  • Waived: $2.00 (charged $0.00)
  • View usage details

Note

This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. Manage usage-based reviews.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between f000c11 and bb4122b.

Files selected for processing: 9
  • changelog.d/fixes/0000-call-log-worker-native-typescript.md
  • src/lib/logEnv.ts
  • src/lib/usage/callLogArtifactWriter.ts
  • src/lib/usage/callLogArtifacts.ts
  • tests/helpers/callLogSettle.ts
  • tests/unit/call-log-artifact-worker-native.test.ts
  • tests/unit/call-log-artifact-worker-tsx-cache.test.ts
  • tests/unit/call-log-artifact-worker.test.ts
  • tests/unit/call-log-save-drain.test.ts
Files with no reviewable changes: 1
  • tests/unit/call-log-artifact-worker-tsx-cache.test.ts

Limit details: You’ve used all 10 included reviews currently available.

@diegosouzapw
diegosouzapw merged commit 9a5a81f into cheaper-inference:release/v3.8.53 Nov 4, 2026
35 of 38 checks passed
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.

2 participants