Skip to content

Make a settled collector rejection local-only instead of retrying it forever - #6

Open
MaxGhenis wants to merge 2 commits into
fix/telemetry-foreign-run-local-onlyfrom
fix/telemetry-settled-rejection-local-only
Open

MaxGhenis wants to merge 2 commits into
fix/telemetry-foreign-run-local-onlyfrom
fix/telemetry-settled-rejection-local-only

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 10, 2026 •

Copy link
Copy Markdown

Problem

CollectorDelivery retried every collector answer it did not recognise:

  • Events (_deliver): 202 acknowledges, 401 drops the collector token, 403 makes the run local-only. Any other status called _defer_retry().
  • Registration (_register): 201 registers, 401 drops the token, 403 and 409 make the run local-only. Any other status called _defer_retry().

So a collector that answers a batch with 422 (an event shape it does not accept), or with 400, 404 or 413, was sent the same batch again without end, and that run's queue never drained. _defer_retry() also sets the service's single _next_attempt_at, which gates every later flush_once, so the wedged run's retry delay held back the service's other runs too.

This is the emitter's copy of the bug fixed in Microcosm by PolicyEngine/microcosm@0e80b0361 (part of PolicyEngine/microcosm#1147, merged 2026-10-09). There the trigger was an older collector answering a blocked event with 422. The collector pinned by this repo's CI (80a2a4ac) has that same schema: its status literal has no blocked.

Stacked on #5. The base branch is fix/telemetry-foreign-run-local-only (at 093efb1); this PR is retargeted to main once #5 merges. It needs #5's _make_local_only(..., caller_dependent=...).

Change

src/microcosm_emitter/telemetry/service/collector.py now equals microcosm#1177's (head 615d84f89, which contains 0e80b0361 and #1177's review fixes) apart from the package path. #5 (at 093efb1) carries #1177's review fixes, so the only difference between #5's file and this one is the #1147 rule below.

  • _permanent_client_error(status): a 4xx other than 408 (timeout) and 429 (rate limit).
  • Events: after the 401 and 403 branches, such a status makes the run local-only with reason collector_rejected_events (LOCAL_ONLY_REJECTED_EVENTS) and a warning that names the status (REJECTED_EVENTS_MESSAGE).
  • Registration: after the 401 and 403 branches, 409 or any other such status makes the run local-only with run_registration_rejected.
  • Both are passed caller_dependent=False: the answer is about the run, so a service applies it to a run it adopted from another build as well as to its own. Events 403 and registration 403 stay caller_dependent=True, as in Keep one build's telemetry login from deciding other builds' runs #5.
  • _make_local_only takes the warning text and hands it to _warn_denied. The warning still prints only for the service's own run.
  • 401 still exchanges the collector token again. 408, 429, 5xx and network errors still retry.

README and a changelog.d fragment describe the rule.

How the collector answers (read for this change)

From PolicyEngine/calibration-diagnostics telemetry-service/telemetry_collector/ (app.py, storage.py, models.py) at the pinned 80a2a4ac:

Request Answer Depends on the caller?
Any body over 1 MiB 413, from a middleware that runs before authentication No
Events for a run the collector does not have 404, checked before the owner No
Events for another user's run, or from an unregistered producer 403 Yes
Events or registration outside the schema 422, after authentication No
Registration of another user's run 403 Yes
Registration by the owner with other metadata 409 No (only the owner gets it)

The new contract test executes the 422 and 404 rows against the real pinned collector in CI's collector job.

Invariants

Each is checked by a test named below. Scope is stated per item; none is claimed beyond it.

  1. Classification. For every status from 100 to 599, _permanent_client_error is true exactly when the status is a 4xx other than 408 and 429. Checked exhaustively (test_only_a_settled_client_error_is_permanent).
  2. One outcome per client error. For a service's own run, for each of the two requests (registration, events), and for every status from 400 to 499 (test_every_client_error_has_one_outcome, exhaustive, fake transport):
    • 401: the run stays pending, the next pass exchanges the token again, and nothing is printed.
    • 408, 429: the run stays pending; nothing is sent until the retry delay has passed, then the same request is sent again.
    • 403: the run goes local-only for the login (run_registration_rejected at registration, collector_authorization_rejected at events), with one warning.
    • Any other: the run goes local-only (run_registration_rejected or collector_rejected_events) with one warning, which for events names the status. Nothing about the run is sent again, even after the retry delay.
  3. Another build's run goes local-only only for an answer about the run, received by the service that decides. In the property test's scenarios (2–4 running builds, up to 30 steps, the fake collector), every local-only of another producer's run is either run_registration_rejected after that service received a 409 or a 422 for that run's registration, or collector_rejected_events after it received a 422 for that run's events. It is never caused by the service's own login. Checked after every step with Keep one build's telemetry login from deciding other builds' runs #5's rule that the deciding service's latest collector answer must be that refusal.
  4. Local-only is final. No request about a run reaches the collector after it goes local-only. Unchanged from Keep one build's telemetry login from deciding other builds' runs #5 and still checked after every step, now with the new reason included.
  5. Orphans are delivered or settled. After every service has gone and Keep one build's telemetry login from deciding other builds' runs #5's two rescuers have run (a first pass under any login, then three passes each as Alice and Bob), each run left pending with queued events is delivered in full, unless the collector refuses the run itself: 409 or 422 at registration gives run_registration_rejected, and 422 at events gives collector_rejected_events. Before this change a 422 run was expected to stay pending and queued.
  6. A settled refusal sets no retry delay. After a pass in which an adopted run is refused (events 422, registration 422, registration 409), a pass with no time elapsed still delivers the service's own new event (test_a_refused_orphan_does_not_hold_back_the_adopters_own_run, three cases).
  7. The warning is the own build's only. A service that makes an adopted run local-only for any of those three answers prints nothing (test_an_orphan_the_collector_refuses_goes_local_only_without_a_warning_here, three cases).

Differential checks.

  • Against Microcosm: the diff of this collector.py and microcosm#1177's at 615d84f89, with the package path substituted, is empty (run locally; Microcosm is not a dependency, so this is not in CI).
  • Against the real collector: tests/test_collector_contract.py::test_a_settled_refusal_makes_the_run_local_only_instead_of_retrying (422 and 404), CI collector job only.

Tests

  • tests/telemetry/test_delivery.py
    • Microcosm's test_a_settled_collector_rejection_goes_local_only_instead_of_retrying, ported, with 413, 400 and 503 added and a check that 408/429/503 are sent again once the delay passes.
    • The same for a rejected registration.
    • The two exhaustive tests of invariants 1 and 2.
    • test_an_expired_collector_token_is_exchanged_again_not_made_local_only: 401 is a 4xx too, so branch order matters.
  • tests/telemetry/test_foreign_runs.py
    • The fake collector can refuse a run's registration with 422 (malformed_run_ids), checked before the owner as the real schema check is.
    • The adopted-orphan test covers events 422, registration 422 and registration 409.
    • The property test draws a malformed run id too; its invariant check and final liveness check follow invariants 3 and 5.
    • Host.flush(..., advance=False) runs a pass with no time elapsed, for invariant 6.
  • tests/test_collector_contract.py: the contract case above.

Verification

  • CI: every check passed on d6958da and 0038e99. On 9d4ee06 the collector job passed, which runs the new contract cases (422 for an event outside the schema, 404 for an unknown run, the latter now seeded with the id-pair key) against the real pinned collector; see this PR's checks for the rest.
  • Review round 1 (GPT-6.1 Sol, at d6958da) asked for changes. Its two blocking findings were Keep one build's telemetry login from deciding other builds' runs #5's later commit 07b2c36 read as deletions through the moving base branch; the rebase settled them, and the four tests it replayed pass. Its non-blocking findings are fixed: the invariant checks use Keep one build's telemetry login from deciding other builds' runs #5's latest-answer rule, the new delivery tests close their deliveries, the changelog no longer promises a status-naming warning for registration, and the 1 MiB statement below gives measured thresholds.

Local, macOS, Python 3.14, at 9d4ee06:

  • Touched test files (test_delivery.py, test_foreign_runs.py, test_module.py): 120 passed (33, 41 and 46). tools/check-quality.sh is clean.
  • Property test at 1,500 examples: 1,500 passing, 0 failing (checks/deep_1500_at_9d4ee06.txt). Each new path was reached: an adopted orphan refused with 422 at registration (9.3% of examples) and at its events (6.7%), and an own run made local-only for collector_rejected_events (2.2%).
  • Mutation pass (20 mutants of the new logic, injected in memory by a pytest plugin, tracked files untouched): 19 of 20 killed by assertion failures, none by errors (mutants/SUMMARY.md). They cover the classifier's bounds and retryable codes, each branch's reason and caller-dependence, the warning text, branch order against 401 and 403, and a settled refusal that also sets a retry delay. The survivor removes _warn_denied's once-only guard, which is equivalent: a run is warned about only as it goes local-only, after which it is never delivered again. Disabling the rule (_permanent_client_error always false) fails 16 tests.

Limits and follow-ups

  • 413 against the real collector is not executed. The fake transport covers it. The collector's body-limit middleware answers on Content-Length without reading the body; whether a client always receives that status was not tested.
  • A legitimate batch can exceed 1 MiB. Batches are capped at 100 events, not by bytes. With every event at near-maximal details (8,150 bytes), a full batch with 500-character ASCII messages is 922,304 bytes, under the limit. It goes over when 51 of the 100 events carry a 500-character non-ASCII message from the Basic Multilingual Plane (1,049,804 bytes), or 23 carry one made of astral characters (1,048,804 bytes); checks/batch_size_bound.py in the evidence folder computes these. Such a run now goes local-only where before it wedged; batching by bytes would deliver it. Follow-up for both packages.
  • 404 after a successful registration (the collector no longer has the run) makes the run local-only, as in Microcosm. Registering again might recover it. Follow-up for both packages.
  • A rejected registration's warning names the login. _register passes no message, so the own-run warning for 409, and now for any other settled 4xx, is the "credential was not accepted" text. That was already so for 409 here and in Microcosm. Follow-up for both packages.
  • The token exchange is unchanged, as in Microcosm: an exchange answer other than 200, 401 or 403 is still retried.
  • microcosm#1177 is still open. If its review changes collector.py, this file should follow.
  • An older service sharing the queue still runs its old logic on shared rows (see Keep one build's telemetry login from deciding other builds' runs #5).

axiom: n/a: telemetry infrastructure

🤖 Generated with Claude Code

@MaxGhenis
MaxGhenis force-pushed the fix/telemetry-settled-rejection-local-only branch from d6958da to 0038e99 Compare October 10, 2026 00:23
MaxGhenis added a commit that referenced this pull request Oct 10, 2026
Review round 1 on #6: the changelog promised a warning naming the status
for a refused registration too, but `_register` passes no message, so the
service prints the existing rejected-credential text for any settled
registration refusal, as it already did for 409. Only a refused batch's
warning names the status.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis and others added 2 commits October 9, 2026 21:51
…forever

Ports PolicyEngine/microcosm 0e80b0361 (microcosm#1147) into the emitter's
copy of the telemetry service, on top of the foreign-run fix (#5).

An events answer other than 202, 401 or 403, and a registration answer
other than 201, 401, 403 or 409, was retried without end. A collector that
answers a batch with 422 (an event shape it does not accept), 404, 413 or
400 therefore wedged that run's queue for good, and the retry delay it set
gates every later delivery pass of the service.

A 4xx other than a timeout (408) or a rate limit (429) on a run's events
now makes the run local-only (`collector_rejected_events`) with one warning
that names the status; the same rule covers a rejected registration
(`run_registration_rejected`). Both are about the run and not the caller,
so a service applies them to a run it has adopted too, without a warning in
its own build. 401 still exchanges the token again, 403 is still about the
login, and 408, 429 and 5xx still retry.

collector.py now equals microcosm#1177's (head 615d84f89, which contains
0e80b0361) apart from the package path.

Tests: Microcosm's delivery test, plus the registration side, every 4xx on
both requests, a 401 re-exchange, and a contract case against the pinned
collector (422 for an event outside its schema, 404 for an unknown run).
The foreign-run tests now expect an adopted run refused with 422, at
registration or at its events, to go local-only, accept that only when
the adopting service's latest answer is that 422 for that run, and check
that a refused orphan sets no retry delay.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1 on #6: the changelog promised a warning naming the status
for a refused registration too, but `_register` passes no message, so the
service prints the existing rejected-credential text for any settled
registration refusal, as it already did for 409. Only a refused batch's
warning names the status.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis
MaxGhenis force-pushed the fix/telemetry-settled-rejection-local-only branch from 7ca9e22 to 9d4ee06 Compare October 10, 2026 01:54
MaxGhenis added a commit that referenced this pull request Oct 10, 2026
… lost run again

Three follow-ups to the settled-rejection rule in the telemetry service's
collector delivery, made in step with Microcosm's copy of this code.

A refused registration printed the "credential was not accepted as a
PolicyEngine organization member" warning, because `_make_local_only`
defaulted to it. That was false for a 409 (the owner registering with other
metadata), a 422 and any other settled 4xx, and for a 403 on a registration
or on events, where the collector had accepted the login and refused it the
run. A rejected registration now names its HTTP status, a 403 names the
Hugging Face user, and the message no longer has a default.

A batch was bounded by count only. A full batch of the largest events
(non-ASCII messages, details at their limit) is over the collector's 1 MiB
request limit, which it answers with 413 for the whole request, and that
made the run local-only. A batch is now the longest run of leading events
whose request body fits, measured with the serialization `_http_post` sends.
An event too large to send alone is sent alone, for the collector to refuse.

A 404 for a run's events means the collector does not have a run this
service registered. The service now registers the run again and sends the
same batch on the next pass; a second 404 with no batch acknowledged in
between makes the run local-only, as before.

Stacked on #6 (0038e99).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant