Skip to content

Conversation

@neekolas
Copy link
Contributor

@neekolas neekolas commented Nov 13, 2025

Add a --streams flag to the forks CLI and pass STREAMS_ENABLED=true to tests to start worker message streams

Introduce a --streams CLI flag, plumb options.streams through to set process.env.STREAMS_ENABLED, expose config.streamsEnabled, and start worker.worker.startStream(typeofStream.Message) in tests when streams are enabled. Update help and README accordingly.

📍Where to Start

Start with main and flag parsing in forks/cli.ts, then review runForkTest for environment propagation and streamsEnabled in forks/config.ts, followed by the test setup in forks/forks.test.ts.


📊 Macroscope summarized 24c219c. 4 files reviewed, 8 issues evaluated, 7 issues filtered, 0 comments posted

🗂️ Filtered Issues

forks/cli.ts — 0 comments posted, 2 evaluated, 2 filtered
  • line 187: console.debug is temporarily overridden to suppress output during cleanForksLogs, but it is restored only on the success path. If cleanForksLogs throws, the restoration line is skipped, leaving console.debug permanently muted for the remainder of the process. Use a try/finally to restore console.debug on all paths. [ Out of scope ]
  • line 193: Per-run fork counting is incorrect and cumulative. getForkCount() returns the total number of files currently in logs/cleaned, and in the test loop this value is added to stats.forksDetected and used to classify the current run. This causes double-counting across runs and marks subsequent runs as having forks even if the current run produced none. Fix by counting only new files produced in the current run (delta since previous count), or by isolating/clearing the cleaned directory per run, or by deriving the count directly from the current run’s output files rather than the global directory total. [ Out of scope ]
forks/utils.ts — 0 comments posted, 4 evaluated, 4 filtered
  • line 5: applyPresetToNode does not validate or clamp the preset ranges. If preset.delayMax < preset.delayMin or preset.jitterMax < preset.jitterMin, the computed delay/jitter can be negative. If preset.lossMin/lossMax are outside 0–100 or lossMax < lossMin, the computed loss can be negative or >100. These values are then passed to node.addJitter(delay, jitter) and node.addLoss(loss), which can produce invalid netem rules or throw at runtime. [ Low confidence ]
  • line 24: validateContainers throws inside the try when !node.ip || !node.veth, but immediately catches its own throw and rethrows a different error. This masks the original, more specific message ("has no IP address") and any other underlying errors, always reporting "is not running". Also, the constructed message mentions IP even when veth is missing, which is misleading. Net effect: incorrect error reporting and loss of diagnostic detail. [ Low confidence ]
  • line 56: startChaos uses preset.interval directly in setInterval without validation. If interval <= 0, negative, NaN, or extremely small, Node will schedule the callback as quickly as possible (effectively 0 ms), causing excessive CPU usage and log spam; NaN coerces to 0 as well. There is no guard to prevent this. [ Low confidence ]
  • line 63: clearChaos only calls node.clearLatency() but startChaos applies both jitter/delay (addJitter) and loss (addLoss). If clearLatency does not clear loss rules, packet loss persists after clearing, violating teardown symmetry and leaving residual effects. [ Low confidence ]
workers/manager.ts — 0 comments posted, 2 evaluated, 1 filtered
  • line 402: Unhandled promise rejection risk: the call to appendFile(...) is fire-and-forget using void and has no .catch or await. If the write fails (invalid path, permissions, disk full), Node may emit an unhandled rejection and the failure is silently ignored, leaving the .env file inconsistent. Add explicit await with try/catch or attach a .catch handler to ensure failures are handled and reported. [ Low confidence ]

Copy link
Contributor Author

neekolas commented Nov 13, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@neekolas neekolas force-pushed the 11-13-add_streams_to_fork_test branch 20 times, most recently from b6f80d1 to 1985c18 Compare November 14, 2025 00:19
@neekolas neekolas force-pushed the 11-07-add_network_chaos_to_fork_tests branch from 1416ee2 to 5bde88d Compare November 14, 2025 02:01
@neekolas neekolas force-pushed the 11-13-add_streams_to_fork_test branch from 1985c18 to 289c08c Compare November 14, 2025 02:01
@neekolas neekolas force-pushed the 11-13-add_streams_to_fork_test branch from 289c08c to 24c219c Compare November 17, 2025 16:21
@neekolas neekolas force-pushed the 11-07-add_network_chaos_to_fork_tests branch from 5bde88d to 02d407e Compare November 17, 2025 16:21
@neekolas neekolas changed the base branch from 11-07-add_network_chaos_to_fork_tests to graphite-base/1574 November 17, 2025 16:24
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