Skip to content

test(the-framework): bind a topic run only once it is up (#1165)#1171

Merged
suleimansh merged 1 commit into
mainfrom
suleimansh/fix/1165-topic-run-scratch
Jul 25, 2026
Merged

test(the-framework): bind a topic run only once it is up (#1165)#1171
suleimansh merged 1 commit into
mainfrom
suleimansh/fix/1165-topic-run-scratch

Conversation

@suleimansh

Copy link
Copy Markdown
Member

Closes #1165.

What it actually was

Not a poll budget, and not the settle() teardown the issue's working theory pointed at. I reproduced the exact CI signature locally and it is a test artifact, not a production bug.

tearDownTopicScratch only removes a scratch whose meta reads done; the test's meta reads running, so that path is a no-op here and could never have produced scratch removed: true. The only other remover is the success path of rehomeTopicRun. So on the failing runs the re-home worked: worktree allocated, history moved, continued run spawned, scratch removed. The missing start was the topic run's own.

Why it went missing: the test seeds the bind the instant onStart returns, which fires a re-home that calls terminate(child.pid) on a child that may still be booting. Under load the SIGTERM lands before the stub reaches its first write, the process dies on the default disposition, and its argv line never appears. One line in the log, the whole budget burnt, 1 !== 2.

Reproduction, and the proof it is fixed

Making the stub take 1.5s to reach its first write reproduces CI exactly on this Mac, where it had never failed:

✖ ... (16086.608166ms)          # CI reported 16311ms
  scratch removed: true
  RECORDED STARTS: [["prompt", ... "--continue-run","--resume-session","sess-xyz"]]   # the CONTINUED run
  scratch events log:
  <no scratch events log>

With this change, that same simulation passes 3/3.

Why the wait is the right fix, not a sleep

A real bind cannot arrive before the run is up: the run writes it into its own event log (recordBind, cli.ts:1540; the dashboard only appends to the control file, which the run reads and then records). So a bind the daemon tails always comes from a live process. The test now models that by waiting for the topic run's start before binding it.

Also in here:

  • the stub installs its SIGTERM handler before its first write, closing the last window where a re-home could kill it silently
  • the test(the-framework): make the re-home failure say why #1166 diagnostic prints the recorded starts — "which spawn is missing" was the first question and it could not answer it, which cost a round of CI
  • the poll ceiling drops 15s -> 10s and its comment now says what it is. Raising it never helped (6s, 30s, 15s all failed identically); it is a backstop, and it has to leave room for the three waits this test now makes inside the 60s per-test timeout

Not changed

No production change. spawnDetached runs process.execPath <bin> directly, so there is no intermediary child that can exit while the run lives on, and the "only tear down when the run has ended" guard the issue asks for already exists as the done check in tearDownTopicScratch.

Test-only, so no changeset. pnpm test in packages/the-framework: 1302 pass, 0 fail.

The #1122 re-home test seeded the bind the instant `onStart` returned, so the
re-home it triggers could terminate a child that had not finished booting. On a
loaded runner that SIGTERM landed before the stub recorded its argv, and the run
looked like it never spawned: a bare `1 !== 2` after the whole poll budget.

A real bind cannot arrive that early. The run writes it into its own event log
(`recordBind`, cli.ts), so a bind the daemon sees always comes from a process
that is already running. The test now waits for that start before binding.

Also: the stub installs its SIGTERM handler before its first write, so a signal
in between cannot take the default disposition and kill it silently; the #1166
diagnostic prints the recorded starts, since "which spawn is missing" was the
first question it could not answer; and the poll ceiling drops to 10s, which is
a backstop rather than a slowness allowance -- 6s, 30s and 15s all failed the
same way, because the re-home either lands in under a second or has gone wrong.

Closes #1165
@suleimansh suleimansh added bug Something isn't working priority: medium Worth doing, not urgent the-framework ♻️ labels Jul 25, 2026
@suleimansh suleimansh self-assigned this Jul 25, 2026
@suleimansh
suleimansh marked this pull request as ready for review July 25, 2026 16:10
@suleimansh
suleimansh merged commit a9c86ba into main Jul 25, 2026
3 checks passed
@suleimansh
suleimansh deleted the suleimansh/fix/1165-topic-run-scratch branch July 25, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Worth doing, not urgent the-framework ♻️

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: the #1122 re-home test loses the bind under load (not a poll budget)

1 participant