fix: three ways a write could look like it succeeded when it didn't - #1
Merged
Merged
Conversation
A request that connected fine and whose mutation the server fully executed was reported as "Failed to connect to <url>: operation timed out". The connection succeeded and the work may be done — saying otherwise sends the caller straight into a retry that double-applies. Split the timeout arm out: it now says the request reached the server and the operation may have completed, so check before retrying. The no-retry-on-timeout policy is unchanged. The catch-all arm stops claiming "Failed to connect" for errors that are neither. `is_timeout_error` lets callers of mutating operations key recovery off the same distinction; tests cover that it survives `.context()` wrapping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…f-succeeds `docs create --parent-folder` is two server calls: the namespaced createDocument, then a separate moveNode that actually places the document. Both failure paths left an orphan at the drive root: - moveNode's result was thrown away with `let _ = ...`, so a failing placement printed "✓ Document created" and the folder it never reached. Now it errors with the document id and the exact `docs move` that finishes the job. - A createDocument timeout aborted the command, but the server had often already created the document. The error now looks the drive up by name and reports the id if it is there — and either way says not to re-run create, which is what produces the duplicates. Also drops the `vars["slug"]` assignment: the mutation declares only $name and $parentIdentifier, so GraphQL variable coercion ignored it. It never placed anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A caller that wants to retry a query with a narrower selection — because an older Switchboard rejects a field this CLI now asks for — had no way to ask whether the server answered at all. The GraphQL errors arrived as a bare `bail!`, indistinguishable from a refused connection, so the only safe retry was a blind one that dials a dead host twice. Carry them in a `ServerErrors` type instead, matching `is_timeout_error`'s existing shape, with the same rendering as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A reducer error or a denial does not fail the job: the operation is still
written, the rest of the batch still applies, and the job still reaches
READ_READY with no error. So `jobs wait` printed `READ_READY` and exited 0 for
a batch one of whose actions was rejected, and `docs apply --wait` inherited
exactly that — the caller was told its write succeeded when part of it had
been thrown away.
The job now carries the per-action outcomes (powerhouse#3027), so select and
report them: a tally on the status line, a line naming each action that did
not apply and why, and a non-zero exit from `jobs wait` when they did not all
apply. `jobs status` reports the same detail but still exits 0 — it is asking a
question, not performing a write. JSON output is the job object as the server
sent it, `result` included; nothing is reshaped.
Against a Switchboard without that fix there is nothing to report, and nothing
to report is not success: absent, null and `{}` results all fall back to the
plain status line and the old exit code. `jobStatus` there declares `result`
non-null and resolves it null, which errors the whole selection — hence the
workaround this replaces — so that one query retries without the field, but
only when the server is the one that answered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A gateway timeout is a successful HTTP exchange, so it fell through to
`bail!("HTTP {status}: {body}")` and `is_timeout_error` said no — there is
no `reqwest::Error` in that chain. That is the most common way a slow
upstream surfaces on a hosted Switchboard, and it meant `docs create`
printed a bare `HTTP 504` with none of the "the document may exist, do not
re-run" guidance the read-timeout path had just been given. Classify
504/408 as maybe-applied, sharing one wording with the socket timeout.
While in the same branch: a GraphQL error can arrive with a non-2xx status
— a query naming a field the schema lacks fails validation, which yoga and
Apollo answer with HTTP 400. Parse the body and report it as `ServerErrors`
so callers keying a narrower-selection fallback off `is_server_error` see
it, instead of an opaque HTTP failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`result.actions` holds one entry per submitted action *that produced an operation* — an action producing none has no entry. So `allApplied: false` can arrive over a set of entries that all applied, and the rejected count, computed as `actions.len() - applied_count()`, came out zero: a job printed a clean-looking `(2/2 applied)` and then bailed with "0 of 2 submitted action(s) did not apply". Both numbers were wrong and the message was nonsense. Keep the server's verdict separate from the entries, say so in the tally when it is the only thing objecting, and word the failure off what is actually known. The count is now labelled "reported action(s)", since the entries were never the submitted count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`is_server_error` fires on any GraphQL error, so a bad job id or an expired token sent `jobs status` round the loop twice — and `map_err(|_| e)` then threw the second, live failure away in favour of the first. Narrow the guard to errors that actually name `JobInfo.result`, and report the fallback's own error with the first one as context instead of discarding it. This also picks up the case the guard used to miss entirely: a schema with no `JobInfo.result` at all fails validation rather than resolution, which arrives as HTTP 400. Now that the client classifies such a body as a server error, those servers get the fallback the doc comment always promised them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
After a `createDocument` timeout the recovery lookup matched any node with `kind == "file"` and the same name, anywhere in the drive. A drive that already held a document called "Q1 Invoice" — in any folder — got told "The create DID take effect ... Do NOT re-run `docs create`", naming an unrelated id, when the create may never have run at all. That is a confident false positive that suppresses the correct retry. Search only the drive root, which is where `createDocument` puts the document and where it still is until the `--parent-folder` move runs, and report the match as something to check rather than as proof. The lookup is also bounded at 10s now: it was issued through the same 120s client against the server that had just failed to answer, so the user could wait a second full timeout before seeing any message at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems with the `--parent-folder` failure path, both of which the
create path twenty lines above had already solved for itself.
The message asserted the document "is sitting at the root of drive X". The
move can fail with exactly the read timeout this branch teaches the create
to distrust, in which case it may have applied — so the same failure mode
got a categorical claim here and a hedged one there. Check
`is_timeout_error` and say what is actually known.
And returning Err skipped the JSON print, so `docs create --parent-folder
... --format json | jq -r .id` produced nothing: the id of a document that
definitely exists survived only as prose inside a stderr error chain. Emit
`{id, folderMoveFailed}` before failing, so automation can recover it.
`--parent-folder` was also missing from all three doc sources; add it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
165 lines of comment removed across the four source files this branch touched. Most of it restated the line below it, re-argued a decision the code already makes, or narrated a test whose name said the same thing. What survives is the handful of facts the code cannot state: that a name match after a create timeout is not proof, that `result.actions` is not the submitted count, that validation errors arrive non-2xx, and the two socket tricks in the client tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
|
Just gave it a test. This PR is a great improvement! Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes to the same failure mode: the CLI calling a write clean when it wasn't.
docs create --parent-folderlost the document.createplaces the document with a separate follow-upmoveNodecall, whose result was discarded withlet _ = ...and which is never reached when the create times out. So the document was created, left at the drive root, and reported as a connection failure — and the obvious retry silently made a duplicate. It now reports the document id, the drive, and the exactdocs movethat finishes the job, and never blindly retries.A read timeout was reported as "Failed to connect". The connection succeeded; only the response timed out, and the server may well have completed the operation. Connect timeouts still retry; read timeouts still do not.
A partly-rejected batch reported total success. A reducer error or a denial does not fail the job — the operation is still written, the rest of the batch applies, the job reaches
READ_READYwith no error — sojobs waitanddocs apply --waitprintedREAD_READYand exited 0 for a batch that partly failed. They now name each action that did not apply and why, and exit non-zero.Notes on that last one:
JobInfo.resultto be populated. Until that ships there is nothing to report, and nothing to report is not success: absent, null and{}results all fall back to today's status line and exit code.jobs wait(and thereforedocs apply --wait) exits 1 when the job's actions did not all apply.jobs statusreports the same detail but still exits 0 — it asks a question, it does not perform a write.--format jsonis unchanged in shape: the job object as the server sent it, now carryingresult.jobStatuson a pre-fix server declaresresultnon-null and resolves it null, which errors the whole selection — that is the workaround this removes — so that one query retries without the field, and only when the server is what answered. ThejobChangessubscription was always safe: pre-fix servers publish{}there rather than null.cargo check,cargo test --bin switchboard(64 pass),cargo fmt --checkandcargo clippy -- -D warningsare all clean. The 9 failures intests/cli_integration.rsare the pre-existing no-local-reactor ones — identical on the commit this branch starts from.🤖 Generated with Claude Code