Skip to content

runner: pending callbacks can block the result drain and exit #71

Open

Description

@gabe-bots

Problem

In runtime/runner.mjs line 49, finish writes the result line and calls process.exit(0) only after stdout drains. While stdout drains, program callbacks that are still pending can run. A callback with an endless loop stops the drain and the exit callback. The host then waits until the CPU limit stops the runner, and the host gets an incomplete result.

This finding came from the Greptile review of PR #67. The PR #67 change is already on main through PR #68, which contains the PR #67 commit. A fix round on PR #67 cannot land, so this issue holds the finding.

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Program returns a large value] --> B[finish writes the result line]
  B --> C[stdout drains in the background]
  C --> D[Pending program callback starts an endless loop]
  D --> E[Drain and exit callback do not run]
  E --> F[CPU limit stops the runner]
  F --> G[Host gets an incomplete result]
Loading

Impact

A program with pending background work and a large return value can hang until the CPU limit. The user then gets an incomplete result instead of the full value.

Expected

After finished becomes true, pending program callbacks must not run. The result line must drain and the runner must exit.

Evidence

Greptile finding (P1), runtime/runner.mjs:49, on PR #67 head 7f9f09bc79: #67 (comment)

Waiting for stdout leaves pending program callbacks active. For example, sleep(0).then(() => { while (true) {} }); return 'x'.repeat(1000000) can start its loop while the result is still draining.

Current main has the same code:

const finish = msg => {
  if (finished) return
  finished = true
  process.stdout.write(lineOf(msg), () => process.exit(0))
}

Proposed solution

  1. Stop the program callbacks in runtime/runner.mjs when finished becomes true. Keep the stdout drain.
  2. Add a test to tests/runner.integration.mjs that runs the Greptile example program.
  3. Keep the result line format and the timeout path the same.

How to verify

Run the tests in tests/runner.integration.mjs. The new test must get the full result line and an exit code of 0 before the CPU limit.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    lens:qualityOwner: Quality (refactors, tests, cost)priority:p2Next up

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions