Skip to content

Set watcher heap default to 8GB and validate startup args#67

Merged
johanrembark merged 1 commit intomainfrom
pr/watcher-heap-startup
Feb 26, 2026
Merged

Set watcher heap default to 8GB and validate startup args#67
johanrembark merged 1 commit intomainfrom
pr/watcher-heap-startup

Conversation

@johanrembark
Copy link
Member

Unify watcher startup heap handling so both service and setup start paths apply a normalized --max-old-space-size value from config.

  • Add shared heap normalization with finite/integer checks and safe clamping
  • Add shared watcher launch arg builders used by service and setup startup paths
  • Ensure setup startup reads per-workspace config watcher.maxOldSpaceSizeMb for consistency
  • Remove hard-coded heap flag from npm watcher script so config-driven startup paths stay authoritative
  • Add startup-path tests that assert normalized heap args for both service and setup routes

Validation:

  • node --test --test-timeout 30000 src/api-watcher-start.test.js src/setup-watcher-start.test.js src/watcher/heap-config.test.js

Unify watcher startup heap handling so both service and setup start paths apply a normalized --max-old-space-size value from config.

- Add shared heap normalization with finite/integer checks and safe clamping
- Add shared watcher launch arg builders used by service and setup startup paths
- Ensure setup startup reads per-workspace config watcher.maxOldSpaceSizeMb for consistency
- Remove hard-coded heap flag from npm watcher script so config-driven startup paths stay authoritative
- Add startup-path tests that assert normalized heap args for both service and setup routes

Validation:
- node --test --test-timeout 30000 src/api-watcher-start.test.js src/setup-watcher-start.test.js src/watcher/heap-config.test.js
Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Set watcher heap default to 8GB and validate startup args

Verdict: Approved

CI is green (Tests ✓, Security Audit ✓). The architecture is clean and the change is well-scoped.

What's good

  • Clean extraction: heap-config.js and watcher-launch.js give the normalization logic a proper home and make both startup paths share a single source of truth.
  • normalizeWatcherHeapMb is solid: handles undefined, NaN, 'Infinity', floats (truncated), and out-of-range values without surprises. The !Number.isFinite() guard is the right check.
  • Dependency injection via spawnProcess: makes the API route unit-testable without any monkey-patching.
  • startWorkspaceWatcher closes logFd on spawn failure: good defensive cleanup in the error path.
  • Test coverage: all three test files cover the main edge cases and the integration path end-to-end.

Minor observations (no changes required)

1. logFd is never closed by the caller in setup-gui.js

startWorkspaceWatcher returns logFd and the test correctly closes it with closeSync(result.logFd), but setup-gui.js never calls closeSync on the returned fd. The parent process holds the fd open indefinitely after child.unref(). This is a pre-existing pattern (the old inline code had the same issue), so not a regression from this PR, but worth a follow-up since the returned value now makes the leak more visible.

2. null input not covered in heap-config.test.js

Number(null) is 0, which is finite, gets truncated to 0, and clamps to WATCHER_HEAP_MIN_MB. The behavior is correct, just not tested. Could add normalizeWatcherHeapMb(null) alongside the NaN/undefined/'Infinity' cases.

3. PR description says "Remove hard-coded heap flag from npm watcher script" but package.json is unchanged on both branches

Not a bug — the npm script never had a flag, and keeping it clean is the right call. The description is just a bit misleading about it being a removal.

None of these block merging. The core logic is correct, well-tested, and improves consistency across both startup paths.

@johanrembark johanrembark merged commit 2c87c3b into main Feb 26, 2026
7 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.

1 participant