connector-runner: heartbeat attribution converges after restart + receive-liveness health surface + image HEALTHCHECKs (#2153, carved out of #2208) - #2355
Conversation
eumemic
added
the
pipeline:v2
Code reviewVerdict: fail
Package tests, Ruff, and mypy pass, but they do not exercise the newly enabled image healthchecks against the concrete connector overrides. |
Code reviewVerdict: NEEDS-CHANGES
Add Evidence: reviewed live head |
Code reviewCanonical review record (reconciler-written, company#383) Verdict: NEEDS-CHANGES at
Canonical record written by the reconciler from the reviewer's structured return (company#383); the reviewer's own prose comment is discussion. |
Code reviewVerdict: fail
The prior finding is fixed for Signal, SMS, WhatsApp, and Matrix, but remains violated for Slack and Telegram. Package tests passed ( |
Code reviewVerdict: NEEDS-CHANGES at
Property: Every active connection in each connector image using the heartbeat healthcheck must transition to healthy after its inbound transport is actually able to receive, while remaining unhealthy before that point. Signal, SMS, WhatsApp, and Matrix now mark readiness. Slack and Telegram pass Failing test / replay: from a fresh checkout of this SHA, create Executed checks: package heartbeat/runner tests |
Code reviewCanonical review record (reconciler-written, company#383) Verdict: NEEDS-CHANGES at
Canonical record written by the reconciler from the reviewer's structured return (company#383); the reviewer's own prose comment is discussion. |
Code reviewVerdict: pass at The standing transport-readiness property is now satisfied: Slack marks the connection serving only after Reviewed all 18 changed files. Checks executed: connector HTTP package |
Code reviewVerdict: pass at The standing transport-readiness property is now satisfied: Slack marks the connection serving only after Socket Mode connects, and Telegram does so only after polling starts; failure and pre-start behavior remain fail-closed. The other shipped inbound connectors also mark readiness at their receive boundary. Reviewed the complete 19-file diff. The connector-http suite passed on rerun ( |
Code reviewVerdict: NEEDS-CHANGES
Suggested remedy: Do not perform this check-then-unlink sequence. The normal Replay git clone https://github.com/eumemic/aios.git /tmp/aios-2355
cd /tmp/aios-2355
git checkout deb7ce210a2f984301f77c5fd3f83cb1d57f4467
cat >/tmp/repro.py <<'PY'
import asyncio, tempfile
from pathlib import Path
from aios_connector_http.runner import HttpConnector
class C(HttpConnector): connector='probe'
async def main():
c=C(base_url='http://example.test', token='token')
with tempfile.TemporaryDirectory() as d:
p=Path(d)/'alive'; p.write_text('owned'); old=p.open('rb')
s=p.stat(); c._heartbeat_owned=True; c._heartbeat_identity=(s.st_dev,s.st_ino)
real=asyncio.to_thread; calls=0
async def interposed(fn,*args,**kwargs):
nonlocal calls
calls += 1
if calls == 2: p.unlink(); p.write_text('operator replacement')
return await real(fn,*args,**kwargs)
asyncio.to_thread=interposed
try: await c._remove_owned_heartbeat(p)
finally: asyncio.to_thread=real; old.close()
print({'replacement_survives':p.exists(),'to_thread_calls':calls})
assert p.exists(), 'unsafe cleanup unlinked replacement installed after identity check'
asyncio.run(main())
PY
uv run python /tmp/repro.pyThe prior readiness finding is fixed: all six concrete inbound connector overrides now call |
Code reviewCanonical review record (reconciler-written, company#383) Verdict: NEEDS-CHANGES at
Canonical record written by the reconciler from the reviewer's structured return (company#383); the reviewer's own prose comment is discussion. |
Path identity validation and unlink cannot be made atomic portably, so retain stale-on-shutdown behavior and only relinquish ownership. This guarantees cleanup cannot unlink an operator replacement inode.
Code reviewVerdict: MERGE-SAFE The prior readiness property is satisfied on |
Code reviewCanonical review record (reconciler-written, company#383) Verdict: MERGE-SAFE at No blocking findings at this head. Canonical record written by the reconciler from the reviewer's structured return (company#383); the reviewer's own prose comment is discussion. |
Code reviewVerdict: fail at
The standing transport-readiness and non-destructive-cleanup properties are satisfied on this head. Package tests passed ( |
Code reviewVerdict: fail at
The only change since the prior failing review at |
Code reviewVerdict: NEEDS-CHANGES at
The healthy, unowned branch calls Suggested remedy: Pass the current payload into the healthy claim and ensure claim publishes the validated bytes before or atomically with making the inode fresh. Add deterministic one-iteration tests for both missing-file and stale-debris healthy claims. Replay git clone https://github.com/eumemic/aios.git /tmp/aios-2355
cd /tmp/aios-2355
git checkout 15c2f106cc5789d5a1546d51741b8fc695ce9207
cat >/tmp/repro_first_publish.py <<'PY'
import asyncio,json,os,tempfile,time
from pathlib import Path
from unittest.mock import patch
from aios_connector_http.runner import HttpConnector,_ConnectionState
from aios_connector_http.healthcheck import read_connection_health
class C(HttpConnector): connector='probe'
async def one(c,p):
done=asyncio.Event()
async def stop(_): done.set(); await asyncio.Event().wait()
with patch('aios_connector_http.runner.asyncio.sleep',stop):
t=asyncio.create_task(c._heartbeat_loop(p)); await done.wait(); t.cancel()
try: await t
except asyncio.CancelledError: pass
async def case(stale):
with tempfile.TemporaryDirectory() as d:
p=Path(d)/'alive'
if stale:
p.write_text(json.dumps({'healthy_connection_ids':['old'],'unhealthy_connection_ids':[]})); old=time.time()-3600; os.utime(p,(old,old))
c=C(base_url='http://x',token='x'); c._discovery_cursor=1
c._connections['current']=_ConnectionState('current','a',serve_status='serving')
await one(c,p)
got=read_connection_health(p); print(stale,got,p.read_text())
assert got==(['current'],[]), f'first fresh publish retained invalid/stale payload: {got}'
async def run():
for stale in (False,True): await case(stale)
asyncio.run(run())
PY
uv run python /tmp/repro_first_publish.pyExecuted package healthcheck/runner tests: 108 passed, including malformed/missing/stale fail-closed paths, all-unhealthy and mixed-sibling attribution, stale reclaim, and destructive replacement-refusal guards. |
Code reviewCanonical review record (reconciler-written, company#383) Verdict: NEEDS-CHANGES at
Canonical record written by the reconciler from the reviewer's structured return (company#383); the reviewer's own prose comment is discussion. |
Code reviewVerdict: NEEDS-CHANGES at
Property: Heartbeat publication and reclamation must never unlink an inode that the process did not create, including replacements installed at an internal staging pathname.
Failing test: Interpose Suggested remedy: Track the created staging inode and only clean it through a mechanism that cannot act on a pathname replacement; add the deterministic interleaving as a regression test. Replay git clone https://github.com/eumemic/aios.git /tmp/aios-2355
cd /tmp/aios-2355
git checkout 5e64d46e783faeb543c753471e1b369202662669
cat >/tmp/repro_temp_unlink.py <<'PY'
import os, tempfile
from pathlib import Path
from aios_connector_http.runner import HttpConnector
class C(HttpConnector): connector='probe'
with tempfile.TemporaryDirectory() as d:
path=Path(d)/'alive'
real_link=os.link
staged=None
def interposed(source, destination, *args, **kwargs):
global staged
real_link(source, destination, *args, **kwargs)
staged=Path(source)
staged.unlink()
staged.write_text('operator replacement')
os.link=interposed
try:
identity=C._claim_heartbeat(path, b'{\"healthy_connection_ids\": [], \"unhealthy_connection_ids\": []}', True)
finally:
os.link=real_link
print({'claim_succeeded': identity is not None, 'heartbeat': path.read_text(), 'replacement_survives': staged.exists()})
assert staged.exists(), 'unsafe staging cleanup unlinked a replacement inode'
PY
uv run python /tmp/repro_temp_unlink.pyStanding properties were rechecked: concrete connector transport readiness, non-destructive public-heartbeat cleanup, and valid current payload before freshness are covered by passing executed tests. I also executed malformed/missing/stale fail-closed behavior, all-unhealthy and mixed-sibling attribution, stale reclamation/refusal races, all connector suites, Ruff, and mypy. Live head matched the reviewed SHA. |
Code reviewCanonical review record (reconciler-written, company#383) Verdict: NEEDS-CHANGES at
Canonical record written by the reconciler from the reviewer's structured return (company#383); the reviewer's own prose comment is discussion. |
Code reviewVerdict: fail at
The standing transport-readiness, public-heartbeat cleanup, and first-fresh-snapshot properties remain satisfied. |
An inode identity check cannot make a later pathname unlink safe, so the suggested identity-tracked cleanup would retain the TOCTOU race. Leave hidden staging links/debris in place rather than risk deleting an operator replacement, and cover the post-link replacement interleaving.
Code reviewVerdict: NEEDS-CHANGES at
The replacement-race guard should not be weakened: cleanup must remove only a staging name proven still to identify the inode created by this invocation, without pathname TOCTOU (or use a publication design that does not require an undeletable staging link). Add a normal-claim test asserting no hidden staging entry remains, plus replacement-at-staging refusal coverage. Replay git clone https://github.com/eumemic/aios.git /tmp/aios-2355
cd /tmp/aios-2355
git checkout bc2c9f7c167bb1c8a7caf70951763299d95c4a22
uv run python - <<'PY'
import tempfile
from pathlib import Path
from aios_connector_http.runner import HttpConnector
class C(HttpConnector): connector = 'probe'
with tempfile.TemporaryDirectory() as d:
p = Path(d) / 'alive'
c = C(base_url='http://example.test', token='token')
identity = c._claim_heartbeat(
p, b'{"healthy_connection_ids":["c"],"unhealthy_connection_ids":[]}', True
)
entries = sorted(x.name for x in Path(d).iterdir())
print({'identity': identity, 'entries': entries, 'link_count': p.stat().st_nlink})
assert len(entries) == 1, f'claim leaked staging pathname: {entries}'
PYExecuted at the live head: package heartbeat/runner tests (111 passed), all six connector suites plus Matrix (601 passed, 1 skipped), Ruff, and mypy. Degraded-path coverage included malformed/missing/stale heartbeat refusal, all-unhealthy and mixed-sibling attribution, first-publication snapshots, transport-start failures, stale reclaim, public-path replacement refusal, rollback replacement refusal, and staging-path replacement preservation. The prior fresh-empty/stale-payload, transport-readiness, and destructive-cleanup findings are fixed. |
The healthcheck reader (`_parse_connection_health`) reads the heartbeat without acquiring the writer `flock`, so the in-place `ftruncate(fd, 0)` + `os.write` in `_refresh_heartbeat` made a fresh but EMPTY/partial heartbeat externally visible during every refresh. A concurrent `read_connection_health` / `healthcheck.main()` could observe `([], [])` and the probe exit 1 even though the connection was healthy. This violated the standing requirement that any fresh heartbeat already contain a complete, structurally valid snapshot. Publish atomically instead: populate a fresh `O_TMPFILE` inode with the whole snapshot and `RENAME_EXCHANGE` it into the heartbeat path. An unlocked reader can then only ever observe the prior complete snapshot or the new complete snapshot -- never a torn one. Over-correction guard: a degenerate "never write / never truncate" fix would also make the torn-read test pass. test_refresh_still_replaces_content_positive_control asserts an uncontended refresh really lands the new bytes; the mixed-state test asserts a healthy sibling still publishes. The suggested remedy (publish refresh content so readers see a complete snapshot) is correct as far as it goes, but naively swapping in a new inode on every refresh AMPLIFIES a pre-existing revocation gap: ownership was tracked as (st_dev, st_ino) only, and atomic publication frees the old inode whose st_ino the filesystem can immediately recycle. A paused former owner whose recycled number matched could then resume -- the standing "revoke superseded owner" property. Remedy-as-stated does NOT achieve that standing property. So ownership now also carries a per-generation nonce (an xattr stamped on every published inode); the identity-checked refresh compares it, revoking a stale owner even when the inode number is recycled. The stale-recovery test now asserts that property directly (a former owner cannot refresh) rather than the flaky inode-number-inequality proxy it relied on. connector-http: 183 passed; ruff + ruff format + mypy clean.
Fix round did not completeFix committed at Framing check: The reviewer's framing is ACCURATE but NARROWER than reality. The torn-read defect is exactly as described (in-place ftruncate+write at runner.py:1322-1328, unlocked reader) and reproduces. However, the suggested remedy is INCOMPLETE: the obvious way to 'publish so readers see only complete snapshots' is to swap in a fresh inode, and doing that naively silently breaks a DIFFERENT standing property (revoke superseded owner, established by commit 0ccc831) because freeing the owned inode lets tmpfs recycle its st_ino and a (st_dev,st_ino)-only ownership check can be defeated by a paused former owner. I verified this is a real, measured amplification (collisions made the standing recovery test fail ~1/5) and that a weaker version pre-existed on the original head (3/40). The fix therefore had to be wider than the finding: atomic publication PLUS an inode-reuse-proof ownership nonce. Unverified: I did NOT run the full multi-package 'all connector suites' (reviewer cited 601 passed, 1 skipped) — only the aios-connector-http package suite (183 passed). I did NOT run the GitHub Actions CI jobs themselves to green; I only verified via REST that runs were CREATED and queued/in_progress for the exact head SHA 5b72a84 (Code Validation run 4528, eumemic-bot review, Build signal connector image, Migration head check) — their PASS/FAIL conclusions are pending and UNKNOWN. The ownership-nonce hardening relies on filesystem xattr support (user.* namespace); it is present on the sandbox tmpfs and the code degrades gracefully to (st_dev,st_ino)-only when setxattr/getxattr raise OSError, but I did NOT test the container image's actual heartbeat filesystem (e.g. /var/run) for xattr support, so on a filesystem without xattrs the revocation hardening silently falls back to the pre-existing (no-worse) behaviour. I did NOT exercise the O_TMPFILE-unavailable branch on a real non-supporting filesystem (refresh returns None → relinquish) beyond code inspection. Concurrency reproduction used threads within one process, not true multi-process contention. Attempt 3/3. Automatic retries are TERMINATED; escalated to |
Code reviewVerdict: fail (one blocking convention finding; behaviour is otherwise sound) Reviewed at head What I verified (green)
Blocking finding1. Newly-added test modules do not type-check under the project's strict mypy config.
Non-blocking notes
|
|
Paused — deliberate, not stranded. This PR carries Applying This label has no expiry of its own, so the unpause obligation is recorded in the seat's freeze ledger against the same deadline as the other freeze suppressions (2026-09-06T18:00Z). When the lanes are re-armed, remove |
|
Disposition: HOLD — blocked on a deliberately disabled lane, not stranded. Checked this PR's live state rather than its label:
So no machine will pick this up until the lanes are re-armed. The condition is real and intended. Why this is being re-stated: a Unblock condition (unchanged): when the lanes are re-armed — fix first, then triggers, workflow last — remove |
This PR is suppressed ON PURPOSE until 2026-09-21 — here is the reasoningThe Why it stays pausedThe pipeline freeze is still in force, verified from state rather than history:
Re-arming is blocked on a per-child spend ceiling (aios#2396). Note the correction on that issue: the per-child wall-clock bound already existed and was not the gap — prod carried 10800s, wider than the runaways it was supposed to stop. The consumption ceiling is the real missing piece. Why unpausing now would be worse than leaving itThis PR carries So lifting the pause would not start any work. It would re-expose findings into a queue with no worker — producing recurring alarms nobody can act on, which is precisely how a true alarm decays into furniture. The same mechanism is already visible in aios#2428, where stale What this is notNot a judgement that this PR is fine. Its findings are unreviewed and its state is unknown to me. It is invisible on purpose, and that purpose has an expiry date attached so the invisibility cannot become permanent by default. If the freeze outlives 2026-09-21, the right move is another deliberate extension with fresh evidence — not a longer deadline chosen to make the reminder fire less often. |
This branch has not been deployed
1 participant
Carve-out of #2208
Carried
packages/aios-connector-http/**: runner heartbeat attribution, receive-liveness health surface, healthcheck module, and package tests.python -m aios_connector_http.healthcheck.connectors/matrix/healthcheck.py: the existing matrix Dockerfile invokes this exact path.Dropped
connector.pyedit (Signal, Slack, SMS, Telegram, WhatsApp, Matrix): these are contested readiness-semantics changes and are not needed by the runner health surface.signal/tests/test_envelope_hardening.py,slack/tests/test_serve_connection.py,whatsapp/tests/test_transport_health.py, andmatrix/tests/test_deployment.py.Framing check
All carried Docker HEALTHCHECK commands resolve without any dropped connector implementation: six invoke the carried
aios_connector_http.healthcheckmodule; Matrix invokes the carriedconnectors/matrix/healthcheck.py, which delegates to that module and probes its existing appservice endpoint. Every existing connector suite passes against the carved-out runner.Checks