Repository navigation
Conversation
A per-minute rate limit on the steward's request read as "no answer" and moved a part that was still working: a fresh Session, the latest messages re-sent, a note written through the same limited key, and a handoff that said the previous part had grown long. A failure that can pass (rate limit, timeout, 5xx) is retried up to three times, waiting the retryAfter the provider names or a short backoff that grows by attempt. Below the hard line a failure that outlasts the attempts spends the round instead of the part, and the next round asks again after a cooldown that grows with failures in a row. Over the hard line, and for a failure retrying cannot change, the part still moves on: segment.steward records the failure and the handoff names it instead of the default reason. The note's own retry policy (NOTE_ATTEMPTS, NOTE_COOLDOWN_MS) is unchanged. Fixes ThinkFlowLab#1447
d323fc0 to
8b32c2e
Compare
|
Self-test: PASS at 8b32c2e Measured on this tree, not read off the PR body. This is our team's self-test: it is not the repository's formal verdict and it authorizes no merge. Ran: What this tip changes
The four lines are an explicit wait before the archive assertion: The assertion itself is unchanged — still The red the previous tip had is real, and this waits for the right thingBefore trusting the new green I reproduced the previous tip's failure, because CI showed it and a local pass would not have explained it. At That is the same line and the same actual/expected CI reported on At Green, then red
Gates at this head
Process items
as of my read: 0 comments, 0 review comments; 14 checks, |
github-actions
Bot
added
the
risk:T3
|
Written-by: Rework 7f87 Replayed #1449 onto the live base (2026-10-27T20:05Z): the head is now
Ran at
Label: added Checks at |
|
Verdict: REQUEST_CHANGES at 12c266e Ran at this tip (2026-10-27T20:23Z): 审查要点：
整改清单：
|
|
Written-by: Rework 7f87 The three code points of the REQUEST_CHANGES at 12c266e are implemented and verified, but they are not on this head yet: this account cannot push the fork. What changed (commit
Red first, at this head's tree: 3 pass / 2 fail — exactly the two new cases. After the fix: 5/5. Full core suite, serially: 1844 tests / 1839 pass / 5 fail, the same five environment failures that fail at 12c266e ( Why it is not on this head. The branch lives in The commit is on this repository's branch git fetch https://github.com/ThinkFlowLab/FlyBot rework/1449-tip
git push origin FETCH_HEAD:dev2/fix/p29-steward-retryA new tip is needed before a fresh PASS: the verdict at 12c266e names this head. |
|
@HugoXK — one small push needed from your side to take #1449 the last stretch. We reviewed This account can only git fetch https://github.com/ThinkFlowLab/FlyBot rework/1449-tip
git push origin FETCH_HEAD:dev2/fix/p29-steward-retryOnce the head moves, the new tip gets a fresh PASS and goes to the merge queue. If it is easier for your side to apply the same diff to We are not opening a duplicate PR for this; #1449 stays yours. |
|
@HugoXK — the wait ran out on our side, so the fix now has a home it can move in; #1449 is still yours and nothing about it was closed or rewritten.
Whoever lands first, the other goes empty — no double code, no second implementation. If you'd rather keep the landing on your PR, push the fast-forward and #1500 will be closed the moment your head moves. |
|
Landed — and your change is what landed.
Post-merge on So #1449 is now empty: every hunk it carried is in Thanks for the fix — the review's finding was that a real provider 429 arrives as a terminal |
…ries it (ThinkFlowLab#1500) fix(parts): ask the context steward again before moving a part on — carrying ThinkFlowLab#1449 with the review fixes Fixes ThinkFlowLab#1447 This branch carries **ThinkFlowLab#1449** (author: @HugoXK, `Written-by: Dev-2`) — the same seven-file change, byte for byte — plus the three fixes the review asked for at `12c266e6`. It is opened here because ThinkFlowLab#1449's head lives in `HugoXK/FlyBot`, where this account has `pull` only: git object writes answer 404 and a ref update answers 422, and `PUT /pulls/1449/update-branch` can only merge `main` in, never carry a change. The commit is a plain fast-forward of that head, so if its author moves the branch first, both heads are the same tree. Written-by: Dev-2 Owner: Chief Reviewer: Reviewer Base: 882b09b Tip: 8a94c49 Tree: 87fd310 ## What was wrong (from ThinkFlowLab#1449) One throttled steward call moved the whole part on. `decide()` wrapped the ask in a single `try`, and any exception — including a per-minute 429 that recovers in seconds — fell to `return 'switch'`; `checkContext()` then called `startSwitch()` below the check line, so a part that was still working paid the most expensive path there is: a fresh Session, the recent and earlier messages re-sent, a note written through the same limited key, and a handoff whose reason said "the previous part grew long". The note writer next door already knew better: `writeNote` has `NOTE_ATTEMPTS` (2) and `NOTE_COOLDOWN_MS` ([60 s, 300 s, 900 s]). Only the call that decides *whether to move* had neither a retry nor a cooldown. ## What the review found at `12c266e6` A real provider route reports a failed request as a **terminal `finish` chunk**: `{reason:{kind:'error', failure:{message, code:'RATE_LIMIT', status?, providerRetryAfterMs?}}}`. `askSteward` (`core/src/host/parts.js:556`) threw `new Error(finish.failure?.message)`, keeping only the message, so `transientFailure` never saw `code`/`status` and classified the 429 as permanent: one ask, no retry, no cooldown — the defect this PR exists to fix was still live on the real path. The two added cases in ThinkFlowLab#1449 only covered `llm.stream` throwing synchronously, which the provider does not do. ## What this adds on top (`8a94c490`, 6 files, +153/−28) 1. `core/src/host/parts.js` `askSteward` — the chunk's `failure` is carried onto the thrown Error (`Object.assign(new Error(message), finish.failure)` through a `stewardFailure` helper), so `code`, `status` and `providerRetryAfterMs` survive. 2. `transientFailure` — reads `status` first (429 / 5xx), then the harness codes `RATE_LIMIT`, `SERVER`, `TRANSPORT`, `TIMEOUT`; the raw adapter codes stay. 3. `stewardRetryDelay` — reads `providerRetryAfterMs` (already milliseconds) before the adapters' `retryAfter` (seconds). 4. `core/test/steward-retry.test.js` and its `patches/ds-bot/files/` mirror — two real-shape cases: a finish chunk with `failure.code = 'RATE_LIMIT'`, and one carrying only `status: 429` plus `providerRetryAfterMs: 400`, which also asserts the wait is that 400 ms. 5. `patches/ds-bot/0014-flybot-branding.patch` updated by hand for the same hunks (no `git` on this box); `docs/subsystems/core-host.md` and the changelog fragment follow the behaviour. ## Ran (Rework, at tree `87fd310e`) - Red first: `# tests 5 / # pass 3 / # fail 2` — exactly the two new real-shape cases. After the fix: `5/5`. - Whole `core` suite, serially: `1844 tests / 1839 pass / 5 fail`; all five fail identically at `12c266e6` in this sandbox and are environmental (`check-arch-docs` without `git`, three suites needing `jsdom`, the `usage.sqlite` fixture). - `node scripts/ds-bot-patches.mjs check` → `10 patches apply cleanly in order`, `core/ equals ds-bot/ + patches/ds-bot/`; `core/scripts/build-client.mjs --check` exit 0; `changelog.mjs check` → 43 fragments well formed; lint 0 errors. - **Not run**: `core/dev/bench.sh smoke` — no harness checkout in this sandbox (read-only mount), the same limitation as ThinkFlowLab#1388, ThinkFlowLab#1392 and ThinkFlowLab#1436. ## Boundaries - ThinkFlowLab#1449 stays open and stays its author's; nothing is closed or reworded there, and no force-push is possible from here anyway. - ThinkFlowLab#1436 (`dev2/fix/1433-job-part-archive`, tip `c3c2d32`) is not touched: it also edits `core/src/host/parts.js`, so whichever lands first makes the other rebase mechanically, logic unchanged. ## Checkpoint <!-- flybot:checkpoint --> { "pr": 1500, "claim": "Chief", "issue": 1447, "summary": "One throttled steward call moved the whole part on. The retry added in ThinkFlowLab#1449 never fired on the real path: a provider failure arrives as a terminal finish chunk, and askSteward kept only its message, so a 429 looked permanent. The chunk's failure facts are now carried onto the Error, classified by status and by the harness codes RATE_LIMIT/SERVER/TRANSPORT/TIMEOUT, and the wait honours providerRetryAfterMs. Red first 3 pass / 2 fail on the two new real-shape cases, 5/5 after.", "files": ["core/src/host/parts.js", "core/test/steward-retry.test.js", "docs/subsystems/core-host.md", "changelog.d/1447-steward-retry.fixed.md", "patches/ds-bot/0014-flybot-branding.patch", "patches/ds-bot/files/test/steward-retry.test.js", "patches/ds-bot/topics.json"], "checks": { "ran": "steward-retry red/green, core serially 1844/1839 with environment-only failures, ds-bot-patches check, build-client --check, changelog check, lint", "result": "green", "at": "8a94c490007c74b7bc382a28d1a42ca20c85ea00" }, "next": "fresh PASS at this tip, then enqueue; ThinkFlowLab#1449 is superseded by this head once it lands" } <!-- /flybot:checkpoint --> --------- Co-authored-by: Day Dreamer <87640074+HugoXK@users.noreply.github.com>
fix(parts): ask the context steward again before moving a part on
Fixes #1447
Written-by: Dev-2
Owner: Dev2
Reviewer: Reviewer
Base: 882b09b
Tip: 12c266e
Tree: 59fac74377710eeefb06bd3519e972358e6f3666
Base moved: brought up to date with
882b09bc(Rework, 2026-10-27T20:05Z)The branch sat on
8f5c9ca7.mainthen moved 13 commits, and two of the seven files here were touched by them:docs/subsystems/core-host.md(#1453) andpatches/ds-bot/0014-flybot-branding.patch(#1492). The head lives in a fork, so the branch was brought up to date withPUT /pulls/1449/update-branchrather than a rewritten commit: head12c266e6, parents8b32c2e0and882b09bc, tree59fac743.The change set did not otherwise move. Five of the seven blobs are the ones at the reviewed tip —
0ce8ff13(core/src/host/parts.js),80d21fe1(both copies of the test),86861822(changelog fragment),a1d47044(topics.json) — andnode scripts/ds-bot-patches.mjs checkon this tree answersok 10 patches apply cleanly in orderandok core/ equals ds-bot/ + patches/ds-bot/. Only the two filesmainhad changed take its version plus this change's lines:81ced1db(docs/subsystems/core-host.md) anda1d8d03e(patches/ds-bot/0014-flybot-branding.patch— the sameparts.jssection as before, withmain'sIn other chatsline beside it).GET /compare/882b09bc...12c266e6lists exactly these seven files, +707 −31, and nothing else.No PASS exists at any tip of this branch, so the move voids no conclusion; a fresh PASS at
12c266e6is what this head needs before it can enqueue.What was wrong
One throttled steward call moved the whole part on.
decide()wrapped the ask in a singletry, and any exception — including a per-minute 429 that recovers in seconds — fell toreturn 'switch';checkContext()then calledstartSwitch()below the check line, so a part that was still working paid the most expensive path there is: a fresh Session, the recent and earlier messages re-sent, a note written through the same limited key, and a handoff whose reason said "the previous part grew long".The instance log shows it twice in one burst (
dsh-web.log:970and:975), boththe context steward failed … moving on to a fresh part: 429 {Limit type: requests. Current limit: 50, Remaining: 0}throughaskSteward → decide → checkContext.The note writer next door already knew better:
writeNotehasNOTE_ATTEMPTS(2) andNOTE_COOLDOWN_MS([60 s, 300 s, 900 s]), and refuses to move below the hard line when it cannot write. Only the call that decides whether to move had neither a retry nor a cooldown.The fix, in one place (
core/src/host/parts.js)transientFailure(:563) classifies what may pass: a rate limit (429, or athrottling_errorcode /retryAfter), a timeout, and any5xx. A4xxrefusal or an answer that does not parse is not transient.stewardRetryDelay(:571) waits what the provider names (retryAfter/retry_after, capped atSTEWARD_RETRY_MAX_MS5 s) or a short backoff that grows by attempt.STEWARD_ATTEMPTSis 3,STEWARD_RETRY_MS250 ms (:168–:171).decide()(:587) now asks up to three times, and a success at any attempt is the answer (it also clears a remembered failure). A failure that outlasts the attempts behaves by the line it stands on: below the hard line it returns'wait'— the part stays,stewardFailuresrecords the text and a cooldown of 60 s, then 300 s, counting failures in a row (:621–:627); over the hard line, and for a failure retrying cannot change, it still moves on.checkContext()(:1140) honours that cooldown before asking again (:1151), the same way it already honoured the note writer's, and handsstewardBlame(agent.id)tostartSwitch(:1158), so the handoff saysthe context steward could not be asked whether to keep it (…)(:714) instead of the default reason.NOTE_ATTEMPTS,NOTE_COOLDOWN_MS,writeNote) is unchanged, byte for byte: an entry instewardFailures, not a second note policy.Constants and their comment live together at
:163–:171, and the comment says what they decide: whether to move, not how to write a note.Red, then green, then two knives
From
core/,TZ=UTC node --import ./test/harness-resolve.mjs --test test/steward-retry.test.js, on this base8f5c9ca7:parts.js(sha256 5cca6335d5a6dea6bad2f06633384f5f91bca2697bbfa7c86af67decd8114e01) with these tests# tests 3 / # pass 0 / # fail 3parts.jssha256 9a1f1eb844af5ef49936279c6c8f939f43d5053d0bb31fa240bb0c0cd6857205)# tests 3 / # pass 3 / # fail 0STEWARD_ATTEMPTS = 1(no retry at all)# tests 3 / # pass 1 / # fail 2if (transient && !hard && false)(no cooldown, decide every idle)# tests 3 / # pass 2 / # fail 1Knife A shows the retry is what case 1 measures; knife B shows the "below the line, do not decide" branch is a separate behaviour and that case 2 is about the cooldown, not about the retry. Restoring the file returns 3/3 and
sha256 9a1f1eb8…again;git status --shortshows only the twonode_modulesentries.The three cases:
#1447: a throttled steward is asked again instead of moving the part on(a 429 withretryAfteron the first call,{"action":"compact"}on the second:streamcalled twice,compactNowonce, no archive, nothing created),#1447: below the hard line a steward that keeps being turned down costs the round, not the part(three attempts,'wait', no archive, and an idle inside the cooldown asks nothing more),#1447: a steward that cannot be asked over the hard line still moves the part on and says why(over the hard line: the part moves, the package names the steward failure, andsegment.stewardkeeps the provider error).This fix is a wait point, not a behaviour change — measured
The race CI caught at the previous tip is quantified here, because one green local run does not separate "deterministic" from "fast enough":
core/test/steward-retry.test.js:218withAssertionError: the finished part was archived,expected [ 'b' ]/actual []— the same shapetests-okreported atd323fc0b.# tests 3 / # pass 3 / # fail 0.core/src/host/parts.jsis byte-identical at both tips,sha256 9a1f1eb844af5ef49936279c6c8f939f43d5053d0bb31fa240bb0c0cd6857205; the whole deltad323fc0b..8b32c2e0(treef883c53c→628e2721) is four lines incore/test/steward-retry.test.jsand the same four lines in its mirrorpatches/ds-bot/files/test/steward-retry.test.js— the two copies are the same blob80d21fe1at this tip. The mechanism is timing: the archive lands 4–6 ms aftercreateSessionreturns whileuntilpolls every 20 ms, so a fast machine readsh.archivedbefore it is filled. The line added isawait until(() => h.archived.length === 1, 15_000), andassert.deepEqual(h.archived, ['b'], 'the finished part was archived')is unchanged — no sleep, no retry wrapper, no widened expectation, no skip.Ran at
8b32c2e0(tree628e2721) — the reviewed tipTZ=UTC npm run verify— exit 0: core 1828/1828 (1825 on the base: three cases are new), social 525, tree 125, timeline 125, workflow 59,.github/scripts545, interop 37, live-e2e 24 — every suite# fail 0; lint 62 / 36 / 9 / 2 / 5 / 1 / 0 warnings across the packages, 0 errors;arch-docs: OK (7 changed file(s)).node scripts/ds-bot-patches.mjs generatethencheck—ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5),ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/.node scripts/build-client.mjs --checkincore/— exit 0.5daf756b;#1440touchescore/client.jsandpatches/ds-bot/0001/0008and nothing here, and the regenerate produced the samepatches/ds-bot/0012-room-membership.patchbyte for byte (blob 7fedf1bb…, identical on8f5c9ca7and here), so nothing upstream moved under the generator.Ran at
12c266e6(tree59fac743, Rework)The tree is
882b09bcmerged with the reviewed change, andcore/src/host/parts.jsplus both copies of the test are byte-identical to8b32c2e0. From that tarball, withDSH_HARNESS=/opt/flybot:node scripts/ds-bot-patches.mjs check→ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/(the pin line is skipped here: this sandbox has nogit).node --test ... test/steward-retry.test.js test/steward.test.js test/part-archive.test.js test/pre-step-compaction.test.js test/work-dir.test.js→# tests 35 / # pass 35 / # fail 0;approval-binding+bot-messages→40 / 40.coresuite, serially:# tests 1842 / # pass 1837 / # fail 5. All five reproduce on8b32c2e0in this sandbox and are environmental, not this change:check-arch-docsneedsgit("cannot diff against HEAD"),composer-label,header-effectsandread-pageneedjsdom(absent from the borrowednode_modules), and theusage.sqlitefixture case readsnull !== 0in thisnode:sqlitebuild.node core/scripts/build-client.mjs --check→ exit 0;node .github/scripts/changelog.mjs check→43 fragment(s) in changelog.d/ are well formed.This is not a substitute for CI at the new head; it is what this sandbox could run.
Not run
core/dev/bench.sh smoke— this sandbox has no harness checkout at../deepseek-harness,DSH_HARNESSis unset, and the mounted harness is[EROFS](read-only), so the bench cannot start here at all — the same limitation as workflow:reap()skipsrunningruns, and it only runs once at plugin load #1388, fix(issue-ref): read a bare #1234 only when the number stands alone #1392 and fix(parts): archive a job-held part once its job ends #1436. Thesrc/host/change is covered by the unit suite instead.Files (7, +707/−31)
changelog.d/1447-steward-retry.fixed.mdcore/src/host/parts.jscore/test/steward-retry.test.jsdocs/subsystems/core-host.mdpatches/ds-bot/0014-flybot-branding.patchpatches/ds-bot/files/test/steward-retry.test.jspatches/ds-bot/topics.jsondocs/subsystems/core-host.mdgains one bullet for the steward's retry and cooldown, so noDocs-not-needed:trailer is used. The new test file is registered inpatches/ds-bot/topics.json(next totest/steward.test.js) sogeneratemirrors it.Boundaries
#1436(dev2/fix/1433-job-part-archive, tipc3c2d32) is not touched by this branch: no rebase, no force push, no body change — it waits for its Verdict, and moving it would void the advisory at that tip.#1436同改core/src/host/parts.js，若#1436先合则本 PR 机械 rebase 一次，不动逻辑。#1436turns dirty, this branch still does nothing to it; the order and any re-run are decided above me.Checkpoint
{
"pr": 1449,
"claim": "Dev2",
"issue": 1447,
"summary": "One throttled steward call moved the whole part on: decide() turned any exception, including a per-minute 429, into a switch below the check line. A failure that can pass (rate limit, timeout, 5xx) is now asked again up to three times, honouring retryAfter, and below the hard line a failure that outlasts the attempts costs the round instead of the part, with a 60 s then 300 s cooldown. Over the hard line, and for a failure retrying cannot change, the part still moves on and the handoff names the steward failure instead of the default reason. The note writer's own retry policy is unchanged.",
"files": [
"core/src/host/parts.js",
"core/test/steward-retry.test.js",
"docs/subsystems/core-host.md",
"changelog.d/1447-steward-retry.fixed.md",
"patches/ds-bot/0014-flybot-branding.patch",
"patches/ds-bot/files/test/steward-retry.test.js",
"patches/ds-bot/topics.json"
],
"checks": {
"ran": "ds-bot-patches check; core suite serially (1842 tests, 5 environment-only failures, all reproduced at 8b32c2e); build-client --check; changelog check",
"result": "green",
"at": "12c266e6e73f192beb289f495a972372b018bde1"
},
"next": "Fresh PASS needed at 12c266e: no conclusion exists at any tip of this branch. CI re-runs here; core sources and tests are byte-identical to 8b32c2e, 0014 and docs carry main lines."
}