fix(945): correct five stale comments, narrow the elisp retry handler - #949
Merged
Conversation
jaunder--create-with-retry documented retrying a signalled transport error or a 5xx, but its condition-case caught bare 'error' — so any signalled condition was retried, including the configuration error raised when auth-source holds no entry for the blog. A user with no stored credential waited through two backoffs for a message that was available on the first attempt. Narrow the handler to plz-error, the condition a transport failure actually raises. condition-case matches by hierarchy, so plz-curl-error and plz-http-error are still caught; a configuration error now surfaces at once. The docstring is unchanged — narrowing is what makes it true. The exhaustion test signalled a bare 'error' to stand in for a transport failure, encoding the same wrong model; it now signals a real plz-curl-error with plz's own data shape. A new test pins the first-attempt failure and zero backoff for a configuration error. Refs #945
The module doc said helper arguments are bounded enums, so a call site can never emit an unbounded attribute. Not every helper is enum-derived: atompub_request takes op: &'static str, supplied by atompub_op — a matched-route-plus-method lookup. The invariant the sentence protects holds; the mechanism it names does not cover every case. Adopt the wording docs/ARCHITECTURE.md already carries: bounded enums, or a &'static str from a closed set the call site cannot widen. Refs #945
Four comments in the flow-coverage extractor attributed web.<vertical>.<ident> to the server-fn-tracing gate. The macro derives it: #[macros::server] emits the #[tracing::instrument] with the name (#714). The gate enforces PII discipline — a recordable-type default-deny, a refusal to skip pattern-bound parameters by name, and a refusal of unmodelled attribute arguments — and authors nothing. The forward-matching rationale is untouched: the extractor computes every candidate from the inventory because the naming regime can change again, which is a claim about the code as it stands. ADR-0011 is dropped from these citations for the name; its span-name half is partly superseded by #714. Refs #945
Two more copies of the error #945 found in the extractor comments. Neither was reached by the #927 replay: observability.md is not ARCHITECTURE.md, and ADR-0081 was annotated for other claims but not this one. Both are documents a reader consults before the source, so leaving them would preserve the wrong mental model in the most-read place. observability.md is live prose and is edited. ADR-0081 records a decision, so it gets a dated annotation in the form ADR-0052 already uses; its original bullet stands. The decision the bullet records — match the name forward from the inventory, never invert it — is unchanged, and the regime moving a third time without an extractor edit is evidence for it. Refs #945
Two unrelated stale claims from #945, both one line. The audiences component credited Invalidator::patched for the in-place patch plumbing. Invalidator has no such method; the symbol is the free function client::reactive::patched, called two lines below the comment. flake.nix said "the 7 non-compiling static checks" — there are eight. Rather than correct the number, drop it: this was its third home, and a count in prose that no gate checks is the drift itself. The issue reference stays, and the live list is pub const ALL in tools/devtool/src/check.rs, where the number sits beside the thing it counts. Refs #945
Branch-review follow-ups. jaunder-publish.el reached plz only transitively through jaunder-transport, but the narrowed retry handler now dispatches on the plz-error condition — and condition-case silently never matches a condition symbol no define-error has run for. A transitive require makes that a silent, testable-only-by-luck failure, so require plz where it is used. Also reword the ADR-0081 annotation: it said the regime "moved a third time" while the extractor comment and observability.md both say the repo has had two naming regimes. Both are true of different things (authorship vs name shape) and the juxtaposition read as a contradiction. Refs #945
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
1 participant
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.
Closes #945.
Five in-source comments that described machinery the code no longer has, plus the one real behaviour defect they exposed.
The behaviour fix
jaunder--create-with-retrydocumented retrying "a signalled transport error or a 5xx", but itscondition-casecaught bareerror— so it retried any signalled condition, including the configuration error raised when auth-source holds no entry for the blog. A user with no stored credential waited through two backoffs (~3s) for a message that was available on the first attempt.The handler now catches
plz-error.condition-casematches by hierarchy, soplz-curl-error/plz-http-errorstill retry; a configuration error surfaces at once. The docstring is unchanged — narrowing is what makes it true.The existing exhaustion test stood in for a transport failure with
(error "boom"), encoding the same wrong model; it now signals a realplz-curl-errorwith plz's own data shape. A new test pins one attempt and zero backoff for a configuration error.plzis now required directly rather than arriving viajaunder-transport:condition-casesilently never matches a condition symbol nodefine-errorhas run for, so a transitive require made that a failure mode nothing would catch.The comment corrections
host/src/metrics.rs&'static strfrom a closed set —atompub_request'sopis the second casextask/src/server_fn_coverage/extract.rs×4server-fn-tracingwrites the span name#[macros::server]derives it (#714)docs/observability.mddocs/adr/0081-*.mdweb/src/audiences/component.rsInvalidator::patched— no such methodclient::reactive::patchedflake.nixTwo deviations from the issue as filed, both deliberate:
extract.rshad four stale sites, not three — the module doc carried it too.docs/observability.md, ADR-0081), which the Rebuild ARCHITECTURE.md as the materialized view of the ADR log, and gate it #927 replay did not reach. Leaving them would have kept the wrong model in the two documents a reader consults before the source, so they are folded in. ADR-0081 gets an annotation rather than an edit, per the Rebuild ARCHITECTURE.md as the materialized view of the ADR log, and gate it #927 precedent.On item D, the count was dropped rather than corrected 7→8. This was its third home; a count in prose that no gate checks is the drift itself.
tools/devtool/src/check.rskeeps its count, where the number sits besideALL.No ADR: nothing here is a new decision.
Deferred
#948 — the
posts::api::listingcollision example in the flow-coverage docs (three sites plus ADR-0081) appears unreachable under the macro's placement rule (#358). Found by the branch review, in lines this branch was editing; left out because it needs a real judgement about whether the module check is still load-bearing, not a wording fix.Verification
cargo xtask validategreen, e2e included. Both review axes run: spec conformance found all eleven acceptance criteria delivered with no scope creep; standards found no hard violations.