fix(mount): treat 429 backpressure as a yield, not a terminal cycle failure - #511
Conversation
Closes #509. `agent-relay file` mounts this binary through @relayfile/sdk's Relay CLI surface. Someone who arrived that way installed `agent-relay`, not `relayfile`, so six messages sent them to a binary they do not have: $ agent-relay file writeback status error: credentials not found at ~/.relayfile/credentials.json; run relayfile login --api-key for self-hosted credentials … Help never had this problem — it is rendered from the declared command spec rather than forwarded, so `agent-relay file --help` cannot print `Usage: relayfile …`. Error text is written by the Go binary at runtime and was not covered by that. The binary cannot know how it was reached, so the surface tells it: RELAYFILE_PROGRAM_NAME, set on the child env, read by programName(), which falls back to "relayfile" when unset or blank. Direct users keep seeing the name they typed. Fixed at all five user-facing sites rather than only the reachable two, so a message that becomes reachable later is already correct. The test drives a real spawn through the real resolver with a fixture binary that echoes the variable back, rather than stubbing the spawn: the value has to survive the env the surface actually builds, including when a caller supplies its own. Mutation-verified — dropping the env fails both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-Id: d458bd97-53d8-4f02-be9c-48b67b93c916
Review found the previous commit's fix was a fraction of the problem. It
corrected five sites located by grepping for "run relayfile"; Devin and
Bugbot both pointed out that the same advice appears in other phrasings —
usage lines, setup completion, integration connect, dev guidance, the
supervisor help block, and the nameless branch of the messaging-only error,
which still hardcoded "relayfile setup" while the same sentence's login
advice used programName().
The lesson is that a grep for one phrasing cannot bound this. So:
- 44 `Usage: relayfile …` prints and 38 `usage: relayfile …` errors now
format through programName()
- 8 multi-line raw-string help blocks templated, including the top-level
usage, `listen`, and `supervisor`
- the remaining instruction sites: setup completion, integration connect,
dev guidance, writeback stuck guidance, the login deprecation warning,
and the empty-name setup branch
Deliberately untouched, because they are not instructions:
- `relayfile-listen.service` and `com.relayfile.listen.plist` are real
paths on disk, identical whoever invokes us; a blanket rename would
have broken them
- "delegated relayfile credentials" and "relayfile workspace id" are
nouns describing what is missing
The test asserts the class rather than any message. It runs every usage
printer with the host name set and fails on a pattern, because the leak was
never in one place. It also pins the two service filenames, and that a
direct user still sees "relayfile".
That test immediately found five more leaks than review had named, and then
a sixth: a bare `relayfile` with no verb, in the top-level usage, which both
my pattern and the test's first pattern missed. The test regex now covers a
bare mention too. Mutation-verified in both directions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Session-Id: d458bd97-53d8-4f02-be9c-48b67b93c916
Three suites spawn the binary directly and compare it to the surface. They
exist to prove argv routes through unchanged — but they used whole-output
equality as the proxy, so they failed on the one difference the mount is
supposed to make: mounted, the binary names the host rather than itself.
expected 'Usage: agent-relay file mount [WORKSP…'
to be 'Usage: relayfile mount [WORKSPACE] [L…'
Give the baseline spawn the same RELAYFILE_PROGRAM_NAME the surface sets, so
the comparison isolates routing again. The surface test that asserted the
top-level banner now expects the mounted name, which makes it a stronger
check: it proves both that a leading flag reaches the binary and that the
program name arrived with it.
Not fixed here, and not caused by this branch: client.test.ts's "ErrorEvent
is undefined (Node)" fails identically on origin/main, verified in a clean
worktree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Session-Id: d458bd97-53d8-4f02-be9c-48b67b93c916
…ailure
The workspace Durable Object is single-threaded per workspace, so a busy one
answers `429 workspace_busy` with an advertised Retry-After. That is the server
saying "come back shortly" — the mirror is healthy, the backend is saturated.
The cycle classifier only ever marked `context.DeadlineExceeded` as a yield.
Everything else, 429 included, fell through to a plain cycleOutcomeError, and a
non-yielded error makes the initial bootstrap terminal. So the first cycle of a
cold mount that met a busy workspace died immediately:
mount sync cycle failed: http 429 workspace_busy: workspace durable object is
busy; retry after the advertised delay
failed to start poll mount: initial bootstrap incomplete: initial cycle failed
(0 files synced (authoritative total unavailable)): http 429 workspace_busy
In production that surfaced to Cloud as BootstrapFailedError at elapsedMs≈35000
against budgetSeconds=210 with progressing=false — not a timeout, a refusal to
wait. It accounted for 50 of 62 proactive mount-bootstrap failures over three
days, which in turn were ~half of all proactive run failures.
Cloud-side callers were taught to honour this exact backpressure over
AgentWorkforce/cloud#3507, #3516, #3521 and #3578. This daemon never was, which
is why the failures continued after those merged: across 71 busy failures in
three days, not one carried a retry-exhaustion marker. The failing path had no
retry at all.
A 429 is now a yield, so the bootstrap resumes on the existing ticker inside its
existing budget. No new retry machinery — the budget was always there, the cycle
just had to stop declaring defeat at the first sign of it.
Deliberately narrow: only 429. A 5xx is the server being broken rather than
busy and must stay a real failure, or a genuinely unhealthy backend would look
like a queue and spin the bootstrap for its whole budget instead of failing
loudly. Tests cover both the 429 yield (including through error wrapping) and
the 5xx/403/deadline cases that must NOT be treated as backpressure.
Worth stating: this fixes the symptom. The workspace is busy because a handful
of high-frequency callers hammer one per-workspace singleton, and reducing that
call volume is the other half of the problem.
go test ./cmd/relayfile-mount/ passes; go vet clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ Mounted command hints still leak relayfile
With RELAYFILE_PROGRAM_NAME set, static strings bypass programName() in writebackListUsage and other command paths. Mounted users still receive instructions for the unavailable relayfile binary.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if isBackpressureError(err) { | ||
| lastCycleErr = &cycleOutcomeError{cause: err, yielded: true} | ||
| log.Printf("mount sync cycle yielded to server backpressure (will retry): %v", err) | ||
| return nil |
There was a problem hiding this comment.
🔴 Cold backpressure falsely completes bootstrap
When a cold bootstrap gets 429 before persisting state, isBackpressureError marks it yielded and finishInitialBootstrap returns success. Cloud then accepts an empty mirror as bootstrapped.
Learn more
A cold bootstrap can receive HTTP 429 before saveState publishes a bootstrap block. The new branch records a yielded cycle and returns nil. The caller then invokes finishInitialBootstrap, where readBootstrapResumeState reports inProgress == false. That branch suppresses yielded errors while the root context remains live, then returns nil. No subsequent cycle runs, despite the authoritative private bootstrap state remaining incomplete.
Example: A new workspace receives 429 workspace_busy on its first tree request and writes no state checkpoint. The mount exits 0 with zero files instead of retrying or returning resumable exit 75.
Recommended fix: Before accepting a yielded cycle with no public checkpoint, consult syncer.InitialBootstrapComplete(). If it remains incomplete, keep retrying within the bootstrap budget or return a resumable initialBootstrapIncompleteError; never treat the missing public block as completion.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0ec5909. Configure here.
…tstrap Devin and Cursor both flagged this independently on #511, and they were right. Marking a 429 `yielded` was only half the change. `finishInitialBootstrap` treats a yielded first cycle with nothing in progress as a COMPLETED bootstrap, and that reasoning is sound for the only other thing that yields: a per-cycle deadline cannot reach that state without a persisted checkpoint, so "not in progress" genuinely means finished. A 429 breaks the assumption — it can arrive before the very first saveState, with nothing on disk and nothing synced. The previous commit therefore let a cold `--once` mount that met `workspace_busy` exit 0 and hand Cloud an EMPTY mirror it believed was fully bootstrapped. That is worse than the bug this PR set out to fix. The original failed loudly at 35s of a 210s budget; this would have failed silently, and the agent would have run against an empty tree. The yield now carries a `backpressure` marker distinguishing it from a deadline yield, and a cold backpressure yield returns the resumable incomplete outcome instead of falling through to success. That exits initialBootstrapIncompleteExitCode, so the caller reruns the mount — the correct middle ground between exit 1 (fatal, the original bug) and exit 0 (silently empty, the regression). Where a checkpoint DOES exist the behaviour is unchanged: the existing resume loop owns it. Tests: a 429 yield is distinguishable from a deadline yield in both directions, and the resumable outcome exits retryable under --once while a real failure stays fatal. go test ./cmd/relayfile-mount/ passes; go vet clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
The bug
The workspace Durable Object is single-threaded per workspace, so a busy one answers
429 workspace_busywith an advertisedRetry-After. That is the server saying "come back shortly" — the mirror is healthy, the backend is saturated.The cycle classifier in
cmd/relayfile-mount/main.goonly ever markedcontext.DeadlineExceededas a yield. Everything else — 429 included — fell through to a plaincycleOutcomeError{yielded: false}, and a non-yielded error makes the initial bootstrap terminal.So the first cycle of a cold mount that met a busy workspace died on the spot:
Production impact
This surfaced to Cloud as
BootstrapFailedErroratelapsedMs ≈ 35000againstbudgetSeconds = 210, withprogressing: false. Not a timeout — a refusal to wait, with 175 seconds of budget left unused.Those mount failures were roughly half of all proactive run failures.
Why the earlier fixes didn't help
Cloud-side callers were taught to honour this exact backpressure across AgentWorkforce/cloud#3507, #3516, #3521 and #3578 — executor registration, workflow ACL seeds, web import bulk writes, the clone writer. All TypeScript. This daemon never was.
That's measurable: across 71 busy failures in three days, not one carried a retry-exhaustion marker.
The failing path had no retry at all, which is why the failure rate stayed flat through all four merges (26 / 47 / 33 / 44 / 100 / 34 / 35 / 6 / 20 / 45 per day).
The fix
A 429 is now a yield, so the bootstrap resumes on the existing ticker inside its existing budget. No new retry machinery — the budget was always there; the cycle just had to stop declaring defeat at the first sign of it.
Deliberately narrow: only 429. A 5xx is the server being broken rather than busy and must stay a real cycle failure, or a genuinely unhealthy backend would look like a queue and spin the bootstrap for its whole budget instead of failing loudly. Same for a context deadline, which has its own pre-existing branch.
Tests
workspace_busyis recognised as backpressure, yields, and is still detected through error wrapping (the shape the syncer actually returns).context.DeadlineExceededand a plain error must not be treated as backpressure.go test ./cmd/relayfile-mount/passes;go vetclean.Worth stating plainly
This fixes the symptom. The workspace is busy because a handful of high-frequency callers hammer one per-workspace singleton — the same call-volume problem behind the RelayAuth identity sprawl being addressed in AgentWorkforce/cloud#3819. Reducing that volume is the other half.
🤖 Generated with Claude Code
Note
Medium Risk
Changes mount
--oncebootstrap completion and exit-code semantics on 429, which Cloud uses for readiness; the CLI renames are help-only but touch many user-facing paths.Overview
Mount bootstrap: HTTP 429
workspace_busyis no longer treated as a fatal sync cycle during initial bootstrap. Those errors are classified as server backpressure, recorded as a yielded cycle (like deadline yields), and the poll loop retries within the existing budget instead of failing ~35s in with zero files synced.A cold-start regression is guarded separately: a 429 before any checkpoint is persisted must not fall through as “bootstrap complete.” That case returns a resumable incomplete bootstrap and exits with
initialBootstrapIncompleteExitCode(75) so callers retry rather than accepting an empty mirror or a hard failure. Only 429 counts as backpressure; 5xx and other errors stay terminal.CLI when mounted via
agent-relay file: The relayfile CLI readsRELAYFILE_PROGRAM_NAME(defaultrelayfile) and usesprogramName()everywhere usage lines and “run this next” errors are printed. The TypeScript relay CLI surface sets that env toagent-relay filewhen spawning the binary, with Go and TS tests locking the contract. Systemd/launchd filenames stay literalrelayfile-*.Reviewed by Cursor Bugbot for commit 8f81c20. Bugbot is set up for automated code reviews on this repo. Configure here.