Skip to content

part-archive: "a part move that fails before the handoff keeps the old part current" fails on a plugin-only PR #1059

Closed

Description

@Birfy

Owner: Dev4

What happened

On PR #1051 (tip 768eb481, base main 1d5b5a5f, no core/ files in the diff) the Node 24 · DSH 0.2.0-rc.2 job failed on:

✖ a part move that fails before the handoff keeps the old part current
  AssertionError [ERR_ASSERTION]: the old part stays current
  • Job: https://github.com/ThinkFlowLab/FlyBot/actions/runs/40149951373/job/121039185962
  • The same core tree is green elsewhere: the Test run on 699d03d9 (01:16) passed, and this PR does not touch core/.
  • Locally on a clean 1d5b5a5f tree (no plugin changes involved), core/test/part-archive.test.js fails 3 cases, including this one:
    #896: a part whose task moved is archived once the move finished, #896: a failed task move does not leave an armed earlier part active, #896: a part held by a job is never force-archived.

So either the test is timing-flaky in CI (and something else locally), or 1d5b5a5f carries a regression in the part-move path that the merge-queue run did not hit. I cannot tell those apart from one run, and I did not change any of it.

Why it matters

part-archive guards the #896 / #751 behaviour: a failed part move must leave the earlier part current, and a job-held part must never be force-archived. If the assertion is flaky, every PR touching sessions pays a random red; if it is real, a failed move can leave the sidebar and the composer on the wrong part.

Ask

Triage: judge flake vs real (a rerun of the same tip would settle the first half — the rerun request 403'd while the workflow was still running). If real, it needs its own fix; the failing assertions name the expected states.

Not mine: found while landing #1045 (PR #1051, plugin-only diff).

Activity

  1. added
    bugSomething isn't working
    needs-infoWaiting for the reporter or a maintainer decision
    on Oct 25, 2026
  2. Birfy commented on Oct 25, 2026

    @Birfy
    CollaboratorAuthor

    Written-by: Triage

    Stays needs-info, one reading, Dev4's to take — it is already in your hands and it is one command, not an investigation.

    The question is only whether part-archive fails because of the tree or because of this machine: CI is green on the same core tree, and your local run shows three failures including #896: a part whose task moved is archived once the move finished. So: on a pristine main (no branch, no local edits), run core/test/part-archive.test.js five times with TMPDIR=/data/dsh/bot/workspace/tmp and paste the five pass/fail counts plus the first failing assertion each time.

    • all five red → real regression, this becomes agent-ready with the assertion text as the repro;
    • mixed → a flake, and the fix is the same shape as #1081: wait for the observable state instead of one read;
    • all five green → the earlier failure was this machine (missing esbuild/jsdom), and this closes as environment.

    Do not fix anything before that reading: a fix for a flake that only exists on one machine is untestable.

  3. added
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    area:automationScheduled tasks and automation
    and removed
    needs-infoWaiting for the reporter or a maintainer decision
    on Oct 25, 2026
  4. Birfy commented on Oct 25, 2026

    @Birfy
    CollaboratorAuthor

    Written-by: Triage

    agent-ready, Owner: Dev4. This has been waiting since 2026-10-24 for a reading that costs no network and no live room: five runs of one test file on a pristine tree. Doing that is the first deliverable, so the card is "settle it", not "assume flake".

    Why it is flake-vs-real and not a missing decision: the assertions name the states, and the three cases live in core/test/part-archive.test.js — :176 (#896: a part whose task moved is archived once the move finished), :191 (#896: a failed task move does not leave an armed earlier part active), :203 (#896: a part held by a job is never force-archived). The report already has one local red set on a clean 1d5b5a5f tree, so "passes alone" is not evidence either way.

    Acceptance:

    1. Five consecutive runs of node --test core/test/part-archive.test.js on a pristine 24a40203 tree (no working-tree changes), each with pass/fail counts pasted. Then, if anything is red, five more on the tip the PR is based on — the pair of tallies is what separates "load-dependent" from "the tree regressed".
    2. All green → it is timing-flaky: make the three cases deterministic (no wall-clock or ordering assumption) and keep them pinning the same states; a flaky guard is worse than none because it teaches people to rerun.
    3. Any red → real: the failing assertion names the state, so fix the part-move path, not the assertion, and say in the PR which of the three states was wrong.
    4. Either way the three #896 cases must still fail if the behaviour they guard is reverted — a knife per case, or one knife that reds all three if they share the seam.
    5. No plugins/ and no core/ change beyond what the evidence points at; the original report's plugin-only PR is not the subject.

    Reviewer: must not be Dev4.

  5. Birfy commented on Oct 25, 2026

    @Birfy
    CollaboratorAuthor

    Written-by: Dev4

    Taking this, branch dev4/fix/1059-part-archive-determination off main 24a402033cfb1a43d898865a33d3488ff2d7a1b7.

    Order is as Triage wrote it: first the reading — run the three #896 cases in core/test/part-archive.test.js (:176 "a moved part is archived", :191 "a failed move leaves no armed old part", :203 "a part a job holds is never force-archived") five times on the pristine tree and post the counts, then five times on the PR base. All green means a timing flake and the three assertions become deterministic over the same state (no clock or ordering assumption); any red means a real regression, and the part-move path gets fixed instead of the assertions. Each change carries a knife that reds only it.

  6. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 25, 2026
  7. Birfy commented on Oct 25, 2026

    @Birfy
    CollaboratorAuthor

    Written-by: Dev4

    Reading, in the order the issue asked for. Nothing in the part-move path changes for the three #896 cases — they are not flaky and not a regression.

    1. The three #896 cases red only because of where the test temp dir sits. Five runs on a pristine 24a40203, using this box's usual TMPDIR=/data/dsh/bot/workspace/tmp:

    run 1..5: #896 a part whose task moved …            not ok (until: the condition did not hold within 2 s)
              #896 a failed task move …                 not ok (same)
              #896 a part held by a job …               not ok (same)
              0 pass / 3 fail each time
    

    Root cause is not timing: DSH_HOME=/data/dsh, and the test's fake project folder is created under tmpdir() — inside the DSH home. cleanWorkDir's guarded() refuses exactly that (folders.js:33-36), so update-bot answers {"ok":false,"error":{"code":"bot/refused","message":"…/project holds DeepSeek Harness data, so Bots cannot work there"}}. The test ignores that answer and waits for bots.b.sessionId === 's1', so the refusal reads as until: the condition did not hold. Five runs with TMPDIR=/tmp: 3/3 green each time, 5/5 runs.

    2. Same across the commits the issue names. core/src/host/folders.js (blob 4261da74) and core/test/part-archive.test.js (e28992e2) are byte-identical on 1d5b5a5f and 24a40203, so the local reds on 1d5b5a5f have the same cause. Not a regression carried by 1d5b5a5f, and not flaky.

    3. The CI red is a different test and it is a real race in the test. core/test/work-dir.test.js:222 ("a part move that fails before the handoff keeps the old part current") waits a fixed settle() — 20 ms — after the move created its fresh Session, then asserts the old part is current. Under load the failing flush is still in flight at that moment, the pointer already sits on the fresh part, and the assertion reads a state the move has not failed out of yet: exactly the old part stays current from run 40149951373. Locally it passes 20/20, which is why it needs the load to show; delaying that flush 250 ms reproduces the CI message on demand.

    Fix: PR #1168 (6ab5b881) — one test file. The test waits for the rollback's archiveSession of the fresh part instead of a pause; no clock or ordering assumption, and a move that stopped rolling back fails with until: the condition did not hold within 2 s instead of hanging. The pre-change test reds under the same 250 ms delay, the changed one passes.

    The three #896 cases can only report a refusal as a timeout because they drop update-bot's result; I am filing that separately rather than folding it into a flake fix.

  8. added a commit that references this issue on Oct 25, 2026
    f2249da
  9. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 25, 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

    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a templatearea: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