Repository navigation
Conversation
Port of PolicyEngine/microcosm#1177 (head 662fd4acb). Every service on a host shares one telemetry queue, and CollectorDelivery.flush_once walked every pending run in it with this process's Hugging Face credential. With no credential, or an exchange, registration or events answer of 401/403, it made that run local-only, other builds' runs included. - leases.py: per-producer advisory flock leases in <spool>.leases/, named sha256(run_id NUL producer_id).lock, identical to Microcosm's layout so services from both packages recognise each other's leases. Each service holds its own lease from TelemetryModule.initialize until close; another producer's run is adopted only when its lease can be taken without waiting, and a lease-less run only after 15 minutes idle. - collector.py: _claim before delivery and a re-check of the run's state under the claim; _make_local_only(caller_dependent=...) so answers about the caller's credential decide only the service's own run (registration 403 split from 409); a refused-credential memo; a periodic lease sweep. - module.py: the drain waits only for the service's own run; the lease is released on close and when initialization fails. - spool.py: run_has_deliverable and last_updated (ORM, no migration). - tests/telemetry/test_foreign_runs.py: deterministic tests plus a Hypothesis property test over several producers with mixed logins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The real-collector job builds CollectorDelivery directly; it now takes the service's own registration. The first delivery stands for a service that exits, so it releases its lease before the restarted one starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Microcosm's main has no leases until PolicyEngine/microcosm#1177 merges, so the README, changelog and comments now say from when the two packages' services recognise each other's leases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1 (finding 1, reproduced): a service passed over another build's run under a login the collector refused, and its own run went local-only. With nothing left to deliver, nothing read the login again, so after the login changed to the run's owner the service still skipped the run, comparing against a stale credential fingerprint. Each pass with passed-over runs now notes the credential its requests would use. A session the collector issued stays in use until it expires, as for the own run. Also from the review: - The property test's last two services now start under any login, make a pass, and only then become Alice and Bob, so recovery by a service that was refused first is checked; fresh rescuers hid this. - A local-only decision is checked against the deciding service's latest collector answer, not any earlier one. - test_delivery.py releases the leases its deliveries take. - leases._lock closes its descriptor if the inode check itself raises, so a lock is never left held on a descriptor nobody will close. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
that referenced
this pull request
Oct 10, 2026
…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. The new branches are microcosm#1177's (head 662fd4acb) line for line. collector.py differs from that file, apart from the package path, only by #5's own change that offers a passed-over run again once the login changes. 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>
Author
|
Two more findings from microcosm#1177's review round 1 that apply to this port. The code here at
The lease files are unchanged, so services from both packages still recognise each other's leases. 🤖 Generated with Claude Code |
Ports two fixes from PolicyEngine/microcosm#1177 (1c9bd671b), whose review found them in the code this PR copies. Each has a test that fails on 07b2c36 and passes now. 1. A service that could not lock its own lease still served. hold() gave up on the first lock error, after os.open(O_CREAT) had already made the lease file, and other services read a lease file nobody holds as an exited producer's: they adopted the live run at once and registered it under their own login. hold() now repeats an attempt that errs until its deadline. A delivery that still has no lease raises OwnLeaseUnavailableError where advisory locks exist, so TelemetryModule.initialize prints why and the host exits before readiness; the build goes on without telemetry. A platform with no advisory locks is unchanged. 2. The delivery's per-run memory (_registered, _passed_over, _warned_denied) was keyed by "run_id:producer_id". Both ids may contain colons, so two runs could share a key: a refusal for one passed the other over, and registering one counted for the other. The key is now the (run_id, producer_id) pair. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
that referenced
this pull request
Oct 10, 2026
…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>
This was referenced Oct 19, 2026
This was referenced Oct 28, 2026
Draft
This branch has not been deployed
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
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.
Problem
Every build that supplies the same queue path shares one telemetry spool, and every service's delivery worker walked every pending run in it (
CollectorDelivery.flush_onceoverspool.pending_runs()). For each run it read its own process's Hugging Face credential. With none, it called_make_local_only(registration, missing_huggingface_credential), which permanently setsupload_state = local_onlyon that run, another build's run included. The same applied to a token exchange the collector refused (401/403), a registration refused with 403 or 409, and an events batch refused with 403.So one build started without a credential (a CI shell, a terminal with no
HF_TOKEN, another login) permanently disabled hosted telemetry for every other build's queued events. The new two-process test reproduces it on this repo'smain(6e3cfd1): with the service subprocesses runningmain's code, build B (no credential) leaves build A's runlocal_onlyformissing_huggingface_credential.This is a port of PolicyEngine/microcosm#1177 (head
615d84f89), which fixes the same bug in Microcosm's copy of this service. The lease files are identical, so services from both packages that share one spool recognise each other's leases.How the collector authorizes (read for this change)
From PolicyEngine/calibration-diagnostics
telemetry-service/telemetry_collector/atmain(280459c8).app.pyis identical at the80a2a4acpinned by this repo's collector job;storage.pythere differs only in how it materializes blocked runs.POST /v1/auth/huggingface/exchange: an invalid token gets 401, a non-member ofpolicyenginegets 403, and an unavailable Hugging Face gets 503. Success returns a JWT whosesubis the HF user id.POST /v1/runs: the first registration of a run id records the caller asowner_hf_id(INSERT … ON CONFLICT DO NOTHING, then a locked read). A registration by another HF user gets 403 ("Run belongs to another Hugging Face user"). The owner with different metadata gets 409.POST /v1/runs/{run_id}/events: an unknown run gets 404, checked before the owner. An owner mismatch or an unregistered producer gets 403.So a run belongs to whoever registers it first. Delivering another producer's run with your own credential has three outcomes:
Decision (same as microcosm#1177)
Liveness: per-producer advisory leases, no spool migration
flockon<spool>.leases/<sha256(run_id NUL producer_id)>.lockwhenTelemetryModule.initializebuilds its delivery. That happens before the run is registered, before its started event, and before the host binds the socket, so the run never has an event without a live lease. The service holds the lease for its whole life;TelemetryModule.closereleases it after the drain, and the kernel drops it on any exit, SIGKILL included. A failed initialization releases it too.ORPHAN_IDLE_SECONDS(15 min) without an update. Live services append a heartbeat every 60 s by default.flock,LOCK_EX | LOCK_NB) match microcosm#1177. A test pins the path for a known id pair. A local differential check against microcosm#1177'sleases.pyconfirmed identical paths and mutual exclusion across real processes (below).upload_statevalues and reason strings are unchanged.Change
leases.py(new):ProducerLeases(hold,try_acquire,sweep),ProducerLeaseandOwnLeaseUnavailableError; microcosm#1177's code apart from imports.hold()repeats an attempt that errs until its deadline. A failed attempt can leave a lease file nobody holds, which other services read as an exited producer's._lockcloses its descriptor if the inode check itself raises.collector.py:CollectorDelivery(spool, registration, …)now knows its own run and holds its lease;close()releases it. Where advisory locks exist, a delivery that cannot take its lease raisesOwnLeaseUnavailableErrorrather than serve without it.flush_onceclaims each run first (_claim): the own run always; another run only under its lease, or under the idle fallback.run_has_deliverable) before sending anything. A pass reads its list of pending runs once, then can wait seconds on the collector; meanwhile a run's own service may have made it local-only and exited, freeing its lease._make_local_only(…, caller_dependent=…)is the one gate. Registration 403 is now split from 409._note_current_credential), so a run passed over under one login is offered again once the login changes. A session the collector issued stays in use until it expires, as for the own run, so the new login counts from then._registered,_passed_over,_warned_denied) is keyed by the(run_id, producer_id)pair. It was keyed by"run_id:producer_id", and ids may contain colons.module.py:EventSpool.run_has_deliverable). Before, every shutdown could wait out its full drain while another build had queued events.closeand wheninitializefails.initializeprints why (OWN_LEASE_UNAVAILABLE_MESSAGE, Microcosm's wording) and re-raises. The host then exits before readiness, and the build adapter falls back to running without telemetry, as for any failed start.spool.py:run_has_deliverableandlast_updated, both SQLAlchemy ORM queries.hypothesisjoins the dev dependency group.Differences from microcosm#1177
0e80b0361). Here an events answer other than 202/401/403, and a registration answer other than 201/401/403/409, still defer and retry. So the only caller-independent local-only in this service is a registration 409, and an adopted run whose batch the collector refuses with 422 stays queued for retry. That behavior is unchanged by this PR; Make a settled collector rejection local-only instead of retrying it forever #6, stacked on this branch, ports #1147's rule.TelemetryModule.initializerather than amain(), and the drain isTelemetryModule.close. A service that cannot take its lease failsinitialize(the host exits before readiness) where Microcosm'smain()returns 1.database=None, so no.hypothesis/directory lands in checkouts.initializetoclose; a failedinitializereleasing it; an adopted run's events 403 passed over; a passed-over run offered again after a login change, at once with no session and after expiry with one; a lease whose inode check fails not left locked; runs whose ids join alike kept apart (refusal and registration); a lock refused once at startup not exposing the run;hold()waiting out refused locks; a module, and a real service, that cannot take the lease not starting.Invariants
Each is checked by
tests/telemetry/test_foreign_runs.py, within the fake collector's model of the hosted one. The first five are also checked after every step of a Hypothesis property test (150 examples in CI) over 2–4 running builds plus up to 30 random steps:run_registration_rejected; a run whose batches get 422 stays pending and queued.test_delivery.pycases pass with only the new constructor argument. Thetest_module.pyfixture now closes the real delivery before replacing it with a mock, so its lease is not leaked.Verification
All local runs below are on
093efb1, macOS, Python 3.14.7.tests/telemetry/test_foreign_runs.py,test_delivery.pyandtest_module.py: 95 passed.ruff checkandruff format --checkare clean.main's code. Two tests ran with their service subprocesses onmain(6e3cfd1); both pass on this branch.{'build-a': 'missing_huggingface_credential'} != {'build-a': 'pending'}.assert not True), sincemainhas no leases.leases.pybeside this one:no_own_leasehad errors: with no lease ever granted, every service now refuses to start, so the 46test_module.pyfixtures error. It is also killed by assertion failures.original_bugno_gate_make_local_onlygateno_claimgate_all_foreignconflict_caller_dependentno_recheckno_passed_over[403, 403, 403] != [403]) and at events, similar ids, session wait-outno_credential_memono_credential_refreshrefresh_ignores_session[403, 403] != [403])hold_gives_up_on_errorserve_without_own_leaseDID NOT RAISEjoined_run_keys"run_id:producer_id")no_own_leaseOwnLeaseUnavailableErrorat start; lease exclusivity and sweep assert) and 46 fixture errorsalways_idledrain_everythingmain's drain over every runclose_keeps_leaseclosefailed_start_keeps_leasemain'sinitializefailure pathsweep_heldtrust_replaced_fileunchecked_lease_leaklease_name_without_nulwarn_for_foreign07b2c36and on each earlier head except the first: quality, tests on Linux and macOS with Python 3.13 and 3.14, the package build, and the real collector against PostgreSQL.231a26afailed only in the collector job, becausetests/test_collector_contract.pybuiltCollectorDeliverywithout the newregistrationargument;0e6328dfixed that test.Evidence (scripts, logs, mutant plugin and results, review prompts and reports) is in a local folder,
~/reviews/microcosm-emitter-foreign-local-only-2026-10-09/.Review
Round 1 (GPT-6.1 Sol, at
94ca5c1) returned REQUEST_CHANGES with one should-fix and two nits. The reviewer could not execute (free disk was under its floor), so each was checked here first.07b2c36addresses all three.94ca5c1and pass now. The property test's last two services now start under generated logins before becoming Alice and Bob, so it covers this path; with the fix removed it fails.maindoes for the own run, and a test pins it.test_delivery.pyleft its deliveries' leases held; a fixture now closes them.Also from its notes:
leases._locknow closes its descriptor if the inode check itself raises.Round 2 (Sol, at
07b2c36) approved with no findings. It executed the recovery path across refused logins, aNonelogin, session expiry and 401s, plus all 25 rescue-login pairs and a differential of the own run's requests againstmain.After that, the review of microcosm#1177 found two more defects in the code this PR copies.
093efb1ports both (see Change), each with a test that fails on07b2c36:Round 3 is a delta review of
07b2c36..093efb1.Limits and follow-ups
fcntl. There are no leases there: a service adopts only idle runs and has no exclusion. The host already needs Unix sockets, so no supported platform lacksfcntl.615d84f89apart from the #1147 lines and the wiring above. If its review changes the design again, this port should follow.packages/provider-telemetrylayout; whichever lands second rebases.axiom: n/a: telemetry infrastructure
🤖 Generated with Claude Code