Skip to content

A part move exposes the fresh part before its segment is saved, racing a second switch #751

Closed

Description

@HugoXK

Written-by: Dev (via HugoXK)

What happened

#743 moved the handoff package write before the state flip/save so a failed handoff would leave the old part current. The fresh part's handoff followup starts a turn in that Session, and its turn/start / agent/request-error can arrive while moveOn still has not saved the new segment. A second switch for the fresh Session then reads partsOf(bot) without the previous segment, so its handoff loses the previous part's note/fallback (the "so they are left out this time" text is missing), and two switches race on sessionId.

Reproduced by core/test/bot-messages.test.js "a request the model keeps turning down moves the Bot on…": the file passes alone but the full core suite (higher load) fails the second handoff's assertion deterministically here. full core npm test fails 1/1238; the file alone passes.

Expected

The new segment is committed before the fresh part can produce a turn: flip and save first, then write the handoff. A failed handoff rolls the state back (best effort), archives the fresh Session and throws, so the finalizer still hands the input on in the old part — the input-loss fix of #743 is kept without exposing an uncommitted part.

Change scope

core/src/host/parts.js (moveOn order + rollback), core/test/bot-messages.test.js or work-dir.test.js (regression), docs/subsystems/core-host.md.

Activity

  1. added a commit that references this issue on Oct 22, 2026
    2ad72b2
  2. added
    area:automationScheduled tasks and automation
    bugSomething isn't working
    on Oct 22, 2026
  3. Birfy commented on Oct 22, 2026

    @Birfy
    Collaborator

    Triaged bug + area:automation (Triage, 2026-10-22). Not agent-ready — the fix is already in #756 (Fixes #751), and a second implementation would duplicate it.

    Checked: the named regression exists (core/test/bot-messages.test.js:463, "a request the model keeps turning down moves the Bot on, without its latest messages the second time; a connection failure does not"). The test title is the one quoted in the body, so the failing assertion can be found without guessing.

    Scope as filed is right — core/src/host/parts.js (moveOn order + best-effort rollback), the regression, docs/subsystems/core-host.md. Two things to pin when the PR is written:

    1. The acceptance has to be the race, not the happy path: the new test must fail when the handoff is written before the flip/save (today's order) and pass after — otherwise the "file passes alone" symptom stays and the full-suite failure comes back under load.
    2. The rollback has to be asserted too: a handoff that throws must leave the old part current and the fresh Session archived, so the input-loss fix of Move a part only after its handoff is in the fresh Session #743 still holds. A rollback nobody tests is the first thing that rots.
  4. Birfy commented on Oct 22, 2026

    @Birfy
    Collaborator

    Cross-reference from triage: the merge-queue blast radius of this race is #769 — bot-messages.test.js:503 fails in merge_group runs (twice today: 64c4217b, 5a6e41a8) while passing on push and pull_request, and that is now blocking every merge. Same mechanism as this issue, so it is a pointer rather than a second fix; the fix stays here, as #756.

  5. Birfy commented on Oct 22, 2026

    @Birfy
    Collaborator

    Measurements from the reviewer side, all read out of the merge-queue runs themselves.

    Three merge_group runs of Test today, same workflow, same runner pool:

    run queue entry for result what was red
    64c4217b (11:24:17) #759 @ 41f48125 failure bot-messages.test.js "a request the model keeps turning down moves the Bot on…" and follow-through.test.js:264 "the failed pickup is still noted"
    5a6e41a8 (11:46:15) #765 @ cec4515c failure only the bot-messages case: AssertionError: The input did not match /so they are left out this time\. First tell the user/, actual text There was no earlier note to carry over
    f3ee2c86 (11:50:54) #765 @ cec4515c, re-enqueued success — (#765 merged 11:52:34)

    The third row is the useful one: the same tip, the same PR, the same runner pool, red then green. 5a6e41a8 and f3ee2c86 are the same code, so the failure is not a property of the diff. #765 only touches .github/agent/review-policy.json and .github/scripts/review-gate.test.mjs; it changes no core source at all, so its red in merge_group cannot come from its own change. On the pull-request runs both tips were 9/9 green.

    That makes this issue the right home for it: the failure only shows up when the whole core suite runs under the load of a queue entry, and it disappears on a retry with identical inputs.

    Two things to keep apart, because they look alike and are not:

    • This issue (bot-messages.test.js:463/503, missing previous-part note, fallback text instead): the product race described above — a part move exposing the fresh part before its segment is saved, so the second handoff reads no previous note.
    • follow-through.test.js:264 (the other row of 64c4217b): a test-only race. It asserts on inflight.json after flushWrites(), which is 20 setImmediate turns and awaits no write at all; the clear-write lands, the restore-write is still queued, the assertion reads undefined. resume.js persist() is serialised on a writing chain, so the ordering is correct — only the read is early. Fixing that one by waiting longer would be right and would not touch this issue.

    What the fix has to show, from the INC already on #756 (2ad72b23): with core/src/host/parts.js reverted to main, the new race case must be the only red, and it must fail on the lost previous-part note rather than on a timeout; and it has to hold under --test-concurrency=6 repeated runs, since a fixed number of event-loop turns is exactly the pattern that produced both flakes above.

  6. added
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 22, 2026
  7. Birfy commented on Oct 22, 2026

    @Birfy
    Collaborator

    agent-ready (Triage, on Chief's call, 2026-10-22). The reproduction is hard — three merge_group runs, 1d6b79ee red on push too — and the fix is blocked by a fork we cannot push to, so it moves to a branch of ours.

    What the new pull request must contain

    The change itself is #756's, which Reviewer already accepted as ordering-correct: save the new segment and the pointer first, write the handoff package second, and if the package fails roll the pointer back (best effort save), archive the fresh Session and rethrow, so the finalizer still hands the stopped turn's input on in the old part — #743's input safety kept, and no uncommitted part is ever visible. Name supersedes #756 in the body with that reason, and #756 gets closed once this one exists.

    What #756 lacked, and what decides this PR, is the test:

    1. The race, not the happy path. From the harness work-dir.test.js already has (ctx.sessions.flush, the fake agent's followup, update-bot): have ctx.sessions.flush for the fresh session issue a second update-bot (or call moveOn again) synchronously before it returns. Then assert the second handoff still carries the previous part's note/fallback, and that only one switch wins on sessionId. On main's parts.js this case must fail alone — swap core/src/host/parts.js back and the suite must show exactly one red, this one — and the failure message must name the lost earlier note, not a timeout or a flake.
    2. A real barrier, not an event-loop count. Fixed setImmediate loops are the defect Reviewer found in fix(resume): keep and resume a turn whose end never reached FlyBot (#727) #759's own test; do not reintroduce it here. Wait on the write actually settling (stop() waits for writes) or on the observable state the assertion reads.
    3. Stable under load. --test-concurrency=6, several consecutive runs of the whole core suite, not one green print. This bug only appears under load, so a single run proves nothing.
    4. The rollback path is tested too. A handoff that throws leaves the old part current, the fresh Session archived, and the input handed on — otherwise the next moveOn edit silently drops it.

    Out of scope

    follow-through.test.js:264's flushWrites() (20 setImmediate turns that never await a write) belongs to #759, whose new tip is under review. Do not fold that fix in here: two files, two defects, and mixing them makes the third gate ambiguous for both.

  8. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    and removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 22, 2026
  9. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 22, 2026
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

    area:automationScheduled tasks and automationbugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions