Give the relay a shared store, an owner, and a lifetime longer than one process by thisisjun786 · Pull Request #8 · thisisjun786/codex-relay-workflow

thisisjun786 · 2026-09-16T04:13:00Z

Base dev. Dependent PR #9 builds on this branch.

Problem

The relay's operating lifecycle was demonstrated with an explicit temporary database and a
narrow driver. For an installed service that is not enough: every participant has to reach
the same durable state, and the owner has to be able to control the daemon. This PR makes
where the state lives, who owns the daemon, and what survives a process boundary into things
the code can answer and prove.

Where the state lives, and which rule chose it

resolve_state_dir applies --state > CODEX_SESSION_RELAY_STATE > XDG_STATE_HOME > the home
default and reports which rule won and with what value, so two participants that disagree can
say why. doctor measures what this process can actually do — directory and database access,
real path, device and inode, a rolled-back write transaction — and constructs no Store while
doing it, so a missing, unreadable or read-only state directory is an answer rather than the
crash that would replace it.

Sameness is proved, not assumed. A store carries a minted identifier, but copying the file
copies the identifier, so store-challenge writes a nonce after the fact and
doctor --expect-store/--expect-inode/--expect-nonce distinguishes proven from unproven and
mismatch. Both non-proven verdicts exit 2. doctor also reports where the transport ledger
will resolve, because it reads the environment rather than --state and a managed launch that
forwarded only the flag would split the two.

Who owns the daemon

ScopeRegistry is keyed on the resolved socket path in the passwd home of the effective uid,
so one operating scope is one entry no matter how the socket is spelled or which environment a
participant inherited. A stopped registration is kept deliberately: a second state directory
serving the same socket is a duplicate registration even when nothing is running, and a scan
that only looked at live records could not see it.

Signalling goes through a ProcessHandle holding a pidfd opened before the identity check, so a
pid verified and then signalled cannot belong to a different process by the time the signal
lands, and there is no os.kill fallback. Ownership is decided from the record's installation,
store and boot markers, which need no live process — a foreign installation whose supervisor
has died still owns its worker. A record with no usable start time is unverifiable, and
stop refuses rather than guessing.

ServiceIntent is what the owner asked for, independent of what is running: start and
restart never enable a service that was turned off, and a disable that lands mid-restart is
honoured rather than overwritten by the replacement that was already in flight.

Surviving the process boundary

RelayDaemon.run stays bounded by construction. Continuation is a supervisor over successive
bounded workers: the daemon lock and the scope claim are taken once and inherited through the
descriptors, so the store, the generations and the claim are untouched across a worker
boundary. That is what carries an assignment past four hours without asking the parent model
anything. Repeated failure backs off and is reported rather than retried silently.

service status groups the live assignments by project, so one supervisor carrying several
repositories is visible, and it reads them through a read-only connection so an offline status
still creates nothing.

Limitation stated rather than papered over

Scheduler fairness is not transport concurrency. The adapter serialises on one worker, so a
stalled call still blocks the one behind it; what is guaranteed is that a struggling parent
stops being handed the rest of the budget. Transport execution isolation is adapter-level and
is not delivered here.

Verification

Relay suite 526 passed, 30 skipped. packages.py builds and installs both packages and runs
both suites. validate.py, contracts.py, check_operations_contract.py, the
scripts/ci/tests unittest suite, secrets.sh and git diff --check all exit 0.

The service tests use real child processes on purpose: a mocked flock proves only that the code
called flock, and exclusion is worth asserting only when a second process actually tries.

No daemon was registered or enabled. Every probe ran under a temporary CODEX_HOME, an explicit
temporary --state and a matching temporary CODEX_SESSION_RELAY_STATE, against a probe-only
socket. The real ~/.codex/hooks.json sha256 is unchanged and the operational relay state was
read only.

Review rounds

Three rounds have been answered on this branch, each with a test that fails against the code as
it stood: socket-alias ownership; identity and foreign-record classification; and most recently
foreign worker protection, an unverifiable worker start time, atomic record replacement,
readiness before start reports success, doctor silently creating a store while counting rows,
the missing project grouping, and the default state directory disagreeing with the scope
registry on how a socket path is spelled.

The relay was proven with an explicit temporary database and a narrow driver, so nothing recorded what an operator is entitled to assume in real use: which store was actually selected, who may start or stop the daemon, and what a stuck delivery means. docs/operations.md states that contract. Every section carries a status, so a reader can tell the agreed behaviour from the shipped behaviour rather than inferring it: the state precedence and reconciliation exist today, the selection diagnostics, service lifecycle and ownership land in this PR, and the delivery phase taxonomy lands in the dependent one. It also records the trap that --state alone does not move the bridge adapter's transport ledger, which reads CODEX_SESSION_RELAY_STATE independently.

The relay required all participants to share one database and gave them no way to check. A path string cannot answer it: symlinks and per-sandbox mounts make equal paths unequal and unequal paths equal. Worse, doctor built a Store before it could describe anything, so on the one host that needed a diagnosis, the diagnosis raised instead. State selection now reports itself. resolve_state_dir returns which of the four rules won and the exact value that won it, and doctor prints that alongside the resolved database, its device and inode, and read/write access measured rather than assumed. Identity is a store_id minted once, plus a store_challenge nonce. The nonce is what makes it evidence: copying relay.sqlite3 copies the identifier too, so an identifier match alone is graded unproven, a nonce written after the copy is proven, and a conflict is a mismatch that exits 2 while still printing the whole diagnosis. Unproven also exits 2, because a caller that asked whether this is the same store must not read exit 0 as yes. Services now builds every dependency on first use. That is what lets doctor answer for a missing or read-only state directory without creating or opening it, and the ack property wires its outbox before publishing the service so record_verdict cannot silently drop the obligation. doctor also reports where the transport ledger will land. bridge_adapter resolves it from CODEX_SESSION_RELAY_STATE independently, so --state alone splits the store from the ledger that carries send idempotency, and that split is now visible instead of silent.

Those two sections described behaviour that now exists, and a status line that lags the code is worse than no status line.

A flock on <state>/daemon.lock proves one daemon per state directory and nothing more. A second installation that picked a different state directory for the same App Server took its own lock and believed it was alone, so two services could serve one host quite happily. Stopping one was no better: the code had a pid, and a pid is not a process. Verifying /proc/<pid> and then calling kill leaves a window where that pid belongs to something else. Ownership now lives in a scope registry keyed by the App Server socket, in a directory neither installation chose: the passwd home of the effective uid, read from the passwd database rather than $HOME so it cannot move with the launch environment. Liveness is the lock, never a recorded pid, so a dead record does not release a scope. The record outlives the process on purpose, because a stopped registration on a different store is still a duplicate registration and a live-only scan would never see it. An environment override would have put the hole back one level down, so it does not silently become the authority: it marks the registry isolated, namespaces the key, and is refused unless --allow-isolated-scope says it was deliberate. Termination goes through a pidfd opened before the identity check and held until the process is gone, so the thing verified and the thing signalled are the same thing. Identity is boot id, start ticks, installation and store together. Where pidfd is unavailable this refuses rather than falling back to kill; a signal that cannot be aimed is worse than none. A zombie counts as gone, or the grace period would wait out a process that had already exited and then report it as still running. Intent is durable and separate from liveness. service.json absent means never configured, which is not enabled; start never writes it; restart re-reads it after stopping so an owner who disables the service mid-restart does not get a replacement. The standalone daemon command takes the same scope claim, or two bounded runs with different state directories would still serve one socket unseen. It does not require the managed service's enable intent, because an explicit bounded run is an operator action.

RelayDaemon.run is bounded by construction and should stay that way, so continuation cannot be a longer loop inside one process. It is a supervisor over successive bounded workers. The supervisor acquires daemon.lock and the scope lock once and inherits them to every worker, because flock belongs to the open file description: the two share one, so the lock holds while either lives and a replacement cannot start beside an orphaned worker. That is also why a shared holder releases by closing and never by LOCK_UN, which would release it for both at once. A worker adopts those descriptors instead of taking a second lock, which the naive arrangement would have deadlocked on immediately. A worker is authenticated before it serves: the token recorded in daemon.json, a device/inode check on each inherited descriptor, and a PR_SET_PDEATHSIG armed in its own bootstrap followed at once by a getppid check, since the signal is not delivered retrospectively and a parent that died first leaves an orphan. An incomplete or unmatched supervised invocation is refused rather than falling back to an unlocked run. Recovery runs before the first worker and sends nothing. An expired lease returns its delivery to held_uncertain for the reconciler to judge, never to queued, because a queued row is eligible to send again on no evidence at all. Two defects in the ownership code landed earlier are fixed here. stop() returned before looking at the worker when the supervisor record was already gone, which is exactly the orphan case it needed to reach; and it cleared workerPid before confirming the exit, losing the only handle a later stop had. It now records the stop request before signalling and keeps each identity until that process is confirmed gone. Separately, a delivery now has to be addressed to its own assignment's endpoint. Membership in allowed_recipients is not sufficient: two assignments on one host may legitimately authorize the same recipient, so a completion belonging to A addressed to B's parent passed the membership check and was still a cross delivery. The direction is checked at enqueue and again inside attempt, before any transport call.

chatgpt-codex-connector · 2026-09-16T05:06:16Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-16T21:31:41.177007Z	`5681e26`	New commits
🔒 Security Review	✅ Completed 2026-09-16T05:12:50.561289Z	`59197a8`	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

ScopeRegistry.key hashed the socket path as supplied, only made absolute. A socket reached through a symlink or a relative alias is the same App Server, but the two spellings hashed differently, so both launches took a scope lock and both served it — which is the precise thing this registry exists to prevent. Resolving before hashing closes it. Found by review on #8.

stop() wrote the stop request before it validated ownership, so a refusal still had an effect: a caller told not_ours or ownership_unverifiable had already left a request that would halt another installation's supervisor at its next boundary. The request is now written only after ownership is established. A record with no recorded start time was treated as ours. Without a start time a pid is just a number and whatever holds it now would pass the check, which is the pid-reuse hazard the check exists for. It now reports unverifiable, and stop refuses on that. The supervisor deadline was checked only before spawning, so a worker started just before it outlived the supervisor by a whole segment — up to an hour with the default. The worker's bound is now clamped to whatever remains of the supervisor's own.

Five findings, one of them caused by my own previous fix. That one first: returning unverifiable as soon as a start time was missing discarded definitive proof. Installation, store and boot each establish a record is foreign on their own, so they are now evaluated first and unverifiable is reserved for the case where nothing proves foreign and process identity still cannot be established. A stopped registration naming a different store was being overwritten. The record deliberately outlives the process so a duplicate stays visible, and then the next claim erased it. claim now refuses a scope already registered to another store and returns that registration, so replacing one has to be deliberate. An unreadable nonce was graded a mismatch, which told an operator two participants use different stores when the truth was that this one could not look. Unreadable is unproven; mismatch is reserved for a readable store where the nonce is absent. start waited for the recorded pid to change, but after a crash the OS can hand the replacement the very pid the stale record names, so a healthy service would have timed out as did_not_report. It now matches a per-launch id the child writes. The supervisor released the scope without shared=True on the exceptional path, which issues LOCK_UN on the descriptor every worker inherited and would let another state directory claim the socket beside a still-running orphan. And a managed launch forwarded --state without the environment variable, so it inherited the store/ledger split the contract warns about; both are now set together.

start matched the child's record on the launch id and the daemon lock, and both of those stay true through supervision's cleanup: the supervisor clears the pid from the record while still inside the lock context, and only releases the lock afterwards. A launch that finished during startup, which a small --max-segments or an already expired deadline produces, therefore spent a moment looking exactly like a healthy one. A caller polling in that moment got ok with pid null and nothing running behind it. The launch-id match now also requires a recorded pid. A finished launch falls through to the ordinary paths instead, so the caller learns the child exited or that the launch never reported, which is what happened. The regression test parks a real child in that exact window: it takes the daemon lock and writes the record once, already cleared, so the parent cannot pass for the wrong reason by catching a live pid first. Against the previous condition it fails with ok true and pid null. A second test drives the same path with a pid in the record so the new requirement is not refusing every launch.

Seven findings, all reachable, all reproduced by a test that fails against the code as it stood. ownership() returned none as soon as the recorded supervisor was gone, before reading the markers that prove a record foreign. stop() deliberately falls through to the worker in that case, because an orphaned worker is exactly what a stop has to reach - so a foreign installation whose supervisor had died handed us its live worker to signal. The installation, store and boot markers come from the record and need no live process, so they are read first and win. _stop_worker accepted a missing workerStartTicks and terminated whatever held the recorded pid. That is the rule the supervisor already has, and the worker was exempt from it: the regression test shows the old code returning "exited" for a worker it could not identify. Missing or unreadable start time is now unverifiable, and unverifiable is refused. daemon.json was rewritten in place at every worker boundary, so a concurrent stop could read the truncated middle, call a running supervisor absent, return not_running and leave its worker to finish the segment. The same window could hide the foreign markers and let a stop request be written for another installation. It is now written to a temporary file and renamed over the old one. start reported success from a record supervise() writes before on_start has run. Recovery happens in on_start and can outlast the poll interval or fail outright on the App Server connection, and every signal start matched on - pid, launch id, daemon lock - was already true. A readyAt written only after on_start returns is now required, so an initialisation that is still running, or that failed, is not a started service. doctor promised to construct no Store and then did, indirectly: the contents block counted rows through services.store, and Store.__init__ opens O_RDWR, enables WAL and runs the whole schema script. Pointing the diagnosis at an empty, legacy or unrelated readable relay.sqlite3 silently turned it into a relay database. Counts now go through a read-only connection, and the test asserts the file is still zero bytes with no sidecars afterwards. service status advertised project grouping in the operations contract and returned none; project_key had no caller outside its own unit test. It now groups the live relationships by project - assignments, active count, parents, issues - read through that same read-only path, so an offline status still creates nothing. The default state directory hashed the socket path as spelled while ScopeRegistry.key canonicalised it. A relative path or a symlink alias for one socket therefore chose a different store, and the registry then refused it as a foreign owner instead of letting it join the service already on that socket. Both canonicalise now. Relay suite 526 passed, 30 skipped. validate.py, contracts.py, check_operations_contract.py and git diff --check all exit 0.

…nswers Six findings, three of them about a failure being reported as an absence. start returned did_not_report and walked away from the child it had launched. A child that is merely slow comes up after the caller was told the start failed, and it holds the daemon lock and the scope claim while it does - against the retry the caller is about to make. The child this call started is now terminated and reaped, and the outcome says which. service.json was rewritten in place. read() treats an unreadable document as not configured, which reads as disabled, and the supervisor re-reads intent at every worker boundary: a reader landing in the truncated middle would stop a service its owner had left enabled. Replaced atomically, like the daemon record. supervise cleared workerPid unconditionally on the way out, including after an exception between spawn and wait. The worker still holds the locks it inherited, so erasing the only identity a stop can aim at left it delivering for the rest of its segment while status reported not_running. The identity is kept unless the child was actually waited on. The restart delay was not clamped to the supervisor's own bound the way a worker segment already is. The regression test shows a 0.01 second deadline waiting 240 seconds. projects treated a query that failed as an inventory that is empty. The file opening is not the question; whether the query answered is. An inventory that could not be read now reports unavailable with its detail. nonce_lookup did the same thing one layer down, and worse: a locked, malformed or momentarily unavailable database opened and then failed the query, and calling that readable turned "we could not look" into "it is not there", which compare_store grades as a definite store mismatch. doctor --expect-nonce would have claimed two participants use different stores on a transient read failure. Relay suite 533 passed, 30 skipped.

thisisjun786 · 2026-09-16T07:56:21Z

코디네이터 · 리뷰 루프를 닫는 방법

부모 코디네이터(01a0a6a7-74de-7d63-b03f-ba8753d9c646)입니다. 브리지가 활성 작업에 메시지를 받지 않아 여기로 남깁니다.

관찰한 사실: head가 여러 번 바뀌는 동안 미해결 지적이 7 → 15 → 8 → 11 → 10 사이를 오갈 뿐 줄지 않습니다. push마다 자동 리뷰어가 전체 diff를 다시 읽고 새 지적을 만들기 때문입니다. 지적 품질은 좋고 실제 결함도 여럿 잡혔지만, 매 push마다 새 전체 재리뷰를 요청하는 한 이 루프는 끝나지 않습니다. 직전 이슈(JUN-102)에서 똑같은 패턴으로 head가 9번 바뀌었습니다.

바꿀 것: 지금부터 push마다 새 전체 재리뷰를 요청하지 마세요. 현재 열려 있는 집합을 닫는 데 집중하세요.

현재 열린 지적을 항목별로 셋 중 하나로 분류하세요.

수정 — 이 PR 범위 안의 실제 결함. 고치고, 수정 전 코드에서 실패하는 테스트를 붙이고, 스레드에 커밋과 재검증 결과를 답글로 달고 해소.
반박 — 틀렸거나 이미 다른 경로로 처리됨. 근거(파일·라인·테스트 출력)를 들어 답글로 반박하고 해소. 근거 없는 반박은 안 됩니다.
분리 — 유효하지만 JUN-103 범위 밖이거나 별도 설계가 필요함. 스레드에 판단과 근거를 적고 해소한 뒤 목록을 보고하세요. 제가 Linear 이슈로 분리합니다. 임의로 이슈를 만들지 마세요.

outdated 스레드도 그냥 두지 말고 셋 중 하나로 처리하세요.

중요: 직전 이슈에서 하위가 지적을 코드로는 전부 고치고도 스레드에 답글을 달지 않아 머지가 막혔고, 제가 코드에서 하나씩 대조해 대신 해소해야 했습니다. 코드만 고치고 스레드를 두면 머지되지 않습니다.

범위는 늘리지 마세요. 새 기능을 더하지 말고 열린 지적만 닫으세요. 애매한 건 3번으로 분리해서 저에게 넘기세요.

보고할 것: 분류 결과(수정 N / 반박 N / 분리 N과 각 근거), 최종 head SHA, 그 head의 dev-gate 결과, 남은 미해결 여부. 머지 조건은 셋입니다 — Ready(비 Draft), 최신 base, 현재 head의 dev-gate 성공, 리뷰 스레드 전부 해소. 맞으면 제가 머지합니다.

참고로 JUN-131(PR #10)이 같은 패키지에서 병렬로 돌고 있고, JUN-103이 먼저 머지됩니다. 그쪽이 나중에 합칩니다.

…d a fresh store service disable wrote enabled=false and only then discovered that the recorded supervisor belongs to another installation. service.json is shared by everything pointed at this state directory and a supervisor re-reads intent at every worker boundary, so the refused disable still shut that supervisor down at its next boundary. A refusal that has an effect is not a refusal. Ownership is established first now, and a foreign or unverifiable owner leaves the intent exactly as it was. stop read the daemon record once and used that worker pid after terminating the supervisor. A supervisor replacing its worker inside that window meant the pid named a worker that had already exited, so stop reported success while the replacement - holding the inherited locks - kept delivering. The record is re-read after the supervisor is gone. restart_delay_for had the same unbounded exponent the pre-send backoff did: a supervisor whose worker fails on every segment keeps counting, and past about a thousand failures the product is an integer too large to convert to a float, so the supervisor died of its own backoff instead of retrying at the cap. On a fresh state directory the probe that builds the service answers from a file that does not exist yet and reports no store id, so the daemon record and the durable scope registration were both written with a null id. ScopeRegistry's mismatch guard needs both ids present, so that registration could later be overwritten by a different store - losing the evidence that two stores served one socket, which is the thing the registry exists to keep. The supervisor now takes the real identity from the store it is about to use anyway. A worker whose PR_SET_PDEATHSIG could not be armed is recorded rather than refused. The getppid check beside it already closes the window that matters here, a supervisor that is ALREADY gone, and refusing outright would make the relay unusable on any host without prctl. What is genuinely lost is the later case: a supervisor crashing mid-segment will not have its worker signalled, so that worker runs to the end of its bounded segment holding the locks. Bounded, but real, and now visible in the record instead of assumed. Relay suite 533 passed, 30 skipped.

The replacement guard I added compared launchId alone, and a direct 'service run' never has one - so two anonymous launches compared equal and the guard handed the replacement's worker and record straight back to the stop it was meant to protect them from. Identity is the launch id together with the supervisor's pid and start time, which differ across a replacement whether or not an id was ever assigned. And when a replacement IS detected, the stop now says so. It protected the new launch but still computed ok from the old launch's outcome, which told a caller the relay was down while it was still delivering. That case is a refusal: replaced_by_new_launch, naming what was left untouched. supervise published readyAt even when on_start had already consumed the whole deadline. The loop exits immediately in that case, so a waiting start() was being handed a service that was already on its way out. Readiness is not published when the bound is spent; start then reports did_not_report and reaps the child, which is what actually happened. Relay suite 647 passed, 30 skipped.

…ment The identity tuple I added compared launch id, pid and start time all at once. A supervisor finishing naturally clears its OWN pid during cleanup while keeping the launch id and the start time, so that ordinary lifecycle update looked like a different launch - and a stop that genuinely stopped the service reported replaced_by_new_launch instead of success. Taken in order of stability now: the launch id when there is one, the start time for an anonymous run, and the pid only for a record that has neither. That still separates two anonymous launches, which is what the tuple was widened for. Relay suite 648 passed, 30 skipped.

_now() records whole seconds, so two anonymous launches starting in the same second share a startedAt - and a replacement acquiring the lock inside that second compared equal to the launch being stopped, which is the case the identity tuple exists to separate. The anonymous branch carries the recorded start ticks alongside; the kernel's start-time counter for a pid does not collide. Socket discovery returned whichever matching directory sorted first. Two stores recording the same socket means a copy or separate explicit-state runs, and choosing between them silently operates on one set of assignments today and the other after a rename. Discovery adopts a single match and refuses an ambiguous one; doctor reports every store claiming the socket and flags the ambiguity, so the caller sees why a database looks unexpectedly empty instead of guessing. Adopting nothing is still the wrong database. It is the visible wrong database, which --state can correct. Relay suite 650 passed, 30 skipped.

Round three answered a contested socket by having discover_store_for_socket return None, on the reasoning that adopting neither candidate is wrong but at least visible. It is not visible. resolve_state_dir cannot tell that None from "nothing found", so it falls through to the canonical directory and the first command that writes there creates a THIRD empty database - after which the canonical-exists branch at the top of the function wins every later resolution and both real stores, with their assignments and pending deliveries, are hidden for good. That is worse than the first-match behaviour it replaced. So the ambiguity travels on the selection instead of being discarded at the bottom of a helper. resolve_state_dir asks stores_claiming_socket directly and branches on the count; discover_store_for_socket becomes the "exactly one" wrapper over that same walk, so there is one implementation of it rather than two. The command line refuses when a selection carries candidates, naming them and the sequence that resolves them. doctor and ack-proof are exempt for opposite reasons: doctor is how an operator learns which store to pass, and ack-proof opens no store at all. An explicit --state or environment override never reaches the check, because both return before discovery runs. The second change is the same kind of mistake in disable. A supervisor that has taken the daemon lock and not yet published its record leaves ownership() with nothing to classify, so the old condition - foreign or unverifiable or foreign markers - was false and the shared enabled=false was written anyway. The starting supervisor reads that at its first worker boundary and exits, which is a refusal that still had its effect. stop() already refuses this; disable did not, and neither did enable. The test is not whether the owner is ours but whether the holder can be attributed to us at all. An owned orphan still counts: a supervisor that died leaving our own worker alive answers none while that worker holds the lock through the inherited descriptor, and refusing there would leave an owner unable to disable their own orphan - which stop() deliberately still reaches. A recorded worker number is not enough on its own, though. It has to be alive, readable and still carrying the start time we recorded, and it has to have a boot id where the host has one, because ownership() answers none at its already-gone branch before its own missing-boot check ever runs. enable's other race - it reads ownership before probing the lock, so a foreign holder can replace an owned supervisor between the two - is older than this and is left recorded rather than folded in.

…tified Review round five, all four in this PR's own service surface. enable classified ownership and then probed the lock, with the intent write outside it. A foreign supervisor that passes its own enabled-intent check and takes the lock between those two steps gets enabled=true written over a disable that happened during the handoff, which leaves it running and eligible to restart. It now decides and writes under the lock exactly as disable does. I had recorded this one as a follow-up after my own audit raised it; a reviewer found it independently, which is enough to say the boundary was in the wrong place. start launched the supervisor and then waited for it to report. Anything that left that wait without a confirmed result - an exception, a Ctrl-C while recovery is still initialising - skipped the abandon that the timeout path already did. The default launcher puts the child in its own session, so no terminal signal reaches it: it would finish starting up, take both locks and serve a launch the caller had been told nothing about. The wait is its own method now, wrapped so every exit that is not a confirmed result stops the child. ScopeRegistry.claim took the scope lock and then wrote the registration. A failed write left the handle open, so the lock went on refusing every later start in the same process on behalf of a registration that was never published. It closes the handle and clears it before propagating. A supervised worker validated the token, the parent, the paths and the inherited descriptors, but never compared the store it had just opened with the storeId its supervisor recorded. A database deleted or atomically replaced between worker segments is a different store, and without the comparison the worker serves it while the supervisor and the scope registration still name the original - so every participant asking whether they agree is told yes. It is refused now, before the descriptor checks. Each has a regression test verified failing against the pre-fix source.

A store older than socket provenance records nothing about which socket it serves, and a directory hash cannot be inverted. If the first post-upgrade command uses a spelling we cannot reconstruct, the legacy-hash comparison misses it, discovery finds no claim, and a fresh canonical database is created beside it - which then wins every later resolution and hides its assignments for good. That is the same permanent loss two contested stores cause, arriving by a different route, and doctor was the only place it showed. Selections about to CREATE a store now carry the unidentified siblings, and the command line refuses with unidentified_state_directory. An existing canonical store returns before the check, because it has already answered the question. The way out is the same --state that resolves a contested socket, and it is needed once. supervise checked the recorded intent before it held anything. A disable landing between that check and the instance lock would start a supervisor its owner had already turned off - and one the disabling caller never examined, because what it classified was the previous holder. The intent is re-read under the lock now, so whoever holds it is the one whose intent decides. Last, the segment bound was tested only at the top of the loop, so a finite run slept one restart delay it had no use for before returning. After repeated failures that delay is the backoff cap, which turned a two-segment run into a five-minute wait for a replacement that was never going to be spawned. The bound is checked after the exit code is recorded, along with the deadline. test_a_clean_segment_resets_the_failure_count grew a fourth segment: the reset it asserts was visible only in the delay after the last worker, which is the delay that no longer exists.

…hings stop already refused to act on a replacement it detected, but the detection was a snapshot. The daemon lock goes free the moment the old supervisor and worker are gone, so a new launch can acquire it after the re-read and publish its own record - and the clearing write then erased the identity of a service that is running, leaving every later status and stop with no handle on it. The write happens under the lock now: holding it means no replacement can start while the stop finalises, and failing to take it means one already did, which is reported rather than overwritten. A lock taken by something that has published nothing is refused for the same reason disable refuses it. A stopped supervisor kept nextRestartAt in its record, so a service that had ended read as though a restart were still scheduled. Cleared with the pid. Last, a store records the socket it serves and the first recording wins, which is right - nothing should silently rewrite it. But an explicit --state or CODEX_SESSION_RELAY_STATE reused with a different App Server is a real disagreement rather than a re-opening: the service claims and serves the new socket while the database goes on attributing itself to the old one, so one installation's assignments can be exposed through another and later discovery still matches the store to a socket it no longer serves. The command line refuses that, with both sockets named. doctor and ack-proof stay exempt, as they are for the other state refusals, because describing the disagreement is how an operator resolves it.

The boot rule was applied to one stale record shape and not the other. When a supervisor's pid is cleared but a worker number remains, ownership already refused a record with no boot id on a host that has one - because _stop_worker validates start ticks only, and a reboot resets those along with the pid space. The already-gone path is exactly as stale: the supervisor's pid is still in the record and the process behind it is gone. It answered none, stop fell through to the worker, and an unrelated process holding the old number after a reboot would have been signalled. Same rule, same reason, now on both paths. The other is the guard I added last round for a replacement publishing during a stop. It compared the re-read record's launch identity, which cannot see the case that actually happens: a replacement that has taken the lock and not yet published leaves the OLD record in place, so the comparison finds no difference and the write went ahead anyway. By that point both processes this stop was acting on are confirmed gone, so nothing it knows about can be holding the lock - which makes the failed acquisition itself the evidence, and the only evidence there is in that window. A lock that cannot be taken after both exits is read as a replacement. That subsumes the published-nothing case the previous commit handled separately, so its narrower check is gone rather than left to disagree with this one.

…indow The finalizing write has now been guarded three ways: compare the re-read launch identity, then also treat a failed acquisition after both exits as a replacement, then also handle an absent record. Each asked what the world looked like at one instant, and a replacement can publish immediately after any instant - so each left a different window, and the third left the one where termination is not confirmed and no record is present. Holding the lock is the only thing that excludes a replacement for the duration of the write rather than at a moment, so that is the condition now: the record is written only while this process holds the daemon lock. A lock it cannot take after both processes are confirmed gone is a replacement. A lock it cannot take while the worker is unconfirmed is most likely that worker, through the descriptor it inherited - and nothing is written, which loses nothing, because the record already names the processes a later stop has to reach. That is exactly what the write would have preserved. daemon_lock_if_free was also mapping every OSError to "someone holds it". flock reports contention with EACCES or EAGAIN; an unreadable directory or an exhausted descriptor table is an operational failure and not a statement about who owns this state directory. Callers act on that answer - stop announces a replacement, disable refuses an owner - so the other errors are raised and reported as themselves. It is the rule ScopeRegistry.claim already applies to the scope lock.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d243e636e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Last commit stopped daemon_lock_if_free reading every OSError as "someone holds this directory", but left opening the lock file inside the same handler - and open() raises EACCES of its own, for a lock file that has become unreadable. So the mistake survived one level down: an unreadable daemon.lock still answered "held", and after a stop had actually terminated both processes that answer became replaced_by_new_launch, which a restart then refuses to work around. Opening the file sits outside the contention handler now. Only flock's own EACCES and EAGAIN mean contention; every other failure, at either call, is operational and is raised as itself.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5681e26517

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

thisisjun786 added 5 commits September 16, 2026 12:53

Mark the state-selection and store-identity contract as shipped

035834c

Those two sections described behaviour that now exists, and a status line that lags the code is worse than no status line.

thisisjun786 marked this pull request as ready for review September 16, 2026 05:06

This comment was marked as resolved.

Sign in to view

thisisjun786 mentioned this pull request Sep 16, 2026

Fix what the relay loop does each tick: binding, durability, fairness, diagnostics, supersession #9

Merged

This comment was marked as resolved.

Sign in to view

thisisjun786 mentioned this pull request Sep 16, 2026

Make a delivered message something the recipient can act on #10

Merged

This comment was marked as resolved.

Sign in to view

thisisjun786 changed the title ~~Let every participant prove which relay store it is using~~ Give the relay a shared store, an owner, and a lifetime longer than one process Sep 16, 2026

This comment was marked as resolved.

Sign in to view

chatgpt-codex-connector Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread packages/codex-session-relay/src/codex_session_relay/service.py

chatgpt-codex-connector Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread packages/codex-session-relay/src/codex_session_relay/service.py

Comment thread packages/codex-session-relay/src/codex_session_relay/service.py

Comment thread packages/codex-session-relay/src/codex_session_relay/service.py

thisisjun786 merged commit 8a15a74 into dev Sep 16, 2026
9 checks passed

This was referenced Sep 17, 2026

Document the state and ownership refusals, and make their printed recovery runnable #18

Merged

Isolate the transport so one recipient cannot hold it against another #19

Merged

Conversation

thisisjun786 commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Where the state lives, and which rule chose it

Who owns the daemon

Surviving the process boundary

Limitation stated rather than papered over

Verification

Review rounds

Uh oh!

chatgpt-codex-connector Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

thisisjun786 commented Sep 16, 2026

코디네이터 · 리뷰 루프를 닫는 방법

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

thisisjun786 commented Sep 16, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 16, 2026 •

edited

Loading