Skip to content

Reproduce the registration races and the lock this package can actually hit - #35

Merged
thisisjun786 merged 27 commits into
devfrom
codex/crw-3-contention-regression
Sep 18, 2026
Merged

thisisjun786 merged 27 commits into
devfrom
codex/crw-3-contention-regression

Conversation

@thisisjun786

@thisisjun786 thisisjun786 commented Sep 18, 2026

Copy link
Copy Markdown
Owner

What this adds

Regression coverage for management-registration contention and process-failure recovery in packages/codex-session-relay: 29 new cases across five modules, plus docs/contention-regression.md, which records what each required scenario is proven by and separates evidence reused from existing tests from evidence added here.

No source change. One finding that would have needed one is reported in the map instead.

A premise the issue got wrong, verified

The issue expected the management-marker and completion-hook landings to have moved this package. git log ff3c69b5^1..6f77ab4 -- packages/codex-session-relay is empty: the first changed six files under skills/, the second changed scripts/ and docs/runtime-install.md. The contention those landings introduce is in what now calls this package — a coordinator that publishes marker facts and can stop between any two of them, and a Stop hook that runs guard-evaluate as a separate process against a store the daemon is writing. That is what the new tests reproduce.

Modules

Module Cases What it adds
test_registration_contention.py 4 A bind arriving after the hook already released an unbound turn, and the pending observation surviving it. Registration racing a generation advance under a barrier, so the marker never claims a registration the store contradicts. Two coordinators binding at one instant.
test_failure_recovery.py 6 A tick interrupted inside its own write transaction, then a new daemon over a reopened store delivering exactly once. The hook reading while the daemon writes. A real SQLite lock timeout, bounded, answering state_unreadable rather than receipt_missing, and the same check deciding on the receipt once the holder lets go. The hold bound releasing into the next generation while the rolling window still counts what the last one spent.
test_multi_parent_isolation.py 7 Two parents in different repositories and Linear projects on one store, acknowledging and recording verdicts at the same instant under a barrier with one connection per thread. Each parent's outbox job bound to its own document. One parent held at its attempt cap or its hourly allowance while the other drains.
test_operational_scale.py 8 Bounded runs resuming across store reopens without duplicating. The four-hour crossing extended past store identity and generations to a dispatched delivery, its acknowledgement and its owed coordination write. One real replacement worker, in a real process, reading the handoff back. A declared load of 6 parents × 12 events measured against ceilings derived from RetryPolicy.
test_regression_map.py 4 The map's own inventories, derived from source: every reused class must exist in the module it is attributed to, and every criterion's clock label must agree with which modules actually wait on something real.

Separating the evidence

Reused evidence is named by class in the map rather than rewritten. Criterion 4 (needs_changes into the next generation; duplicate, out-of-order and late events) adds no test at all: test_anchor_binding.py, test_supersession.py and test_receipts.py already cover it through real entry points, and a fourth version would be volume rather than coverage.

Clock provenance is a column because most of this package moves time by hand and some of it does not. The rule is per module and deliberately coarse: a module spends real time if it reads a real clock or starts a process, and a criterion naming such a module is mixed rather than injected. test_regression_map.py derives that set with an ast scan and fails when the map disagrees — it caught two of my own labels being wrong.

The four-hour work is deliberately two proofs: the scripted-clock crossing shows the supervisor preserving the handoff across nine replacements, and a real worker in a real process for one short segment shows a replacement reading it back. A FakeWorker never opens the store, so the first cannot answer the second, and the map says so.

Scale, stated as a bound

6 parents, 12 events each, 72 events, declared as module constants so a report quotes a number read from source. It drains inside a ceiling derived from the policy, no tick exceeds the per-tick or per-parent send ceiling, the backlog only shrinks, and no parent is drained before another is reached. That is a measurement of this harness on one machine with a clock that never sleeps. It is not a throughput figure and it is not support for unbounded parallel operation.

Reported, not fixed

guard.SQLITE_TIMEOUT documents the lock wait the readiness check may spend and nothing passes it anywhere. Its name occurs once in the repository, its own definition; intent.read_only_connection carries a separate literal of the same value, and guard.lookup_receipt calls that function without a timeout. The wait is real — an exclusive writer makes the read raise after a measured 2.00s — but the number producing it is the literal, so changing the constant would change nothing. Wiring it would also move intent.dispatch_generation_state, which this change has no authority over, so test_failure_recovery.py pins both halves and leaves the decision.

The workflow-restore section is the only non-essential block in the revision direction of report.render_revision, so a tight budget removes it first. CRW-94 owns that.

Validation

  • CRW_PACKAGES_TMPDIR=/var/tmp python3 scripts/ci/packages.py — passed, codex-session-relay 1235 tests, up from a 1206 baseline on dev, no empty collection and no skipped case.
  • python3 scripts/ci/validate.py — exit 0.
  • python3 -m unittest discover -s scripts/ci/tests — exit 0.
  • git diff --check — clean.
  • Every test runs in a temporary state directory and a temporary store. Nothing reaches an operational database, an installed runtime, an MCP registration, or a real socket. No daemon is enabled. Processes the tests start are reaped by the tests.

Why scripts/crw_runtime/components.json is in this diff

components.json records subdirectoryTree as the git tree hash of all of packages/codex-session-relay, so adding files under tests/ and docs/ invalidates it even though no source changed. scripts/ci/contracts.py passes on dev and failed on this branch for that reason alone.

Re-derived through the same path verify-definition checks — crw_runtime.definition.git(["rev-parse", "HEAD:" + subdirectory]) — rather than typed by hand, and the rewrite refuses if the recorded value is not unique in the file. Exactly one line moved:

-      "subdirectoryTree": "bb23de2de76bf439edb370e3e9b3455de1522e09",
+      "subdirectoryTree": "166efcf052a072e2112cc8a49ab915e07c53c1c9",

packageTree and sourceDigest are unchanged, which is the check that no source was touched: both cover src/codex_session_relay only. git diff --name-only origin/dev...HEAD confirms no file under src/ is in this diff. The bridge component's three fields are unchanged too.

python3 scripts/ci/contracts.py now exits 0 with "findings": [].

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 18, 2026

Copy link
Copy Markdown

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-18T02:04:29.928958Z 9a9bfe5 New commits
🔒 Security Review Completed 2026-09-18T01:03:42.587095Z 81f417a PR opened
ℹ️ 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.

devin-ai-integration[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c4363243d

ℹ️ 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".

Comment thread packages/codex-session-relay/tests/test_failure_recovery.py Outdated
@thisisjun786
thisisjun786 merged commit 973e83f into dev Sep 18, 2026
9 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a9bfe5c5b

ℹ️ 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".

Comment on lines +390 to +391
first = self.daemon.tick(now=self.clock.now())
self.assertFalse(first.quiet, "the shared daemon went quiet when one project paused")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify B progresses before archiving A

While A is paused, first.quiet == false only proves that some daemon pass did work; observation or reconciliation can make the tick non-quiet even if B receives no delivery. The test archives A before inspecting B, so the subsequent drain proves only the archived case and a regression where pausing one relationship starves another can still pass. Assert B's sends or delivery state immediately after this tick.

Useful? React with 👍 / 👎.

Comment on lines +118 to +120
and never opens the store, so the crossing shows the supervisor preserving records across
replacements. Whether a replacement worker reads them back is a separate question,
answered by a real process in `test_operational_scale.py` and reported separately.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Stop claiming the real worker reads every preserved record

The real-process case only demonstrates that the worker reads the relationship and the newly queued second delivery: its assertions inspect tick notes, a poll observation, and that queued delivery's state. Nothing makes that worker consume or validate the original acknowledgement or sync-outbox row, while the four-hour crossing uses FakeWorker instances that never open the store and merely checks those rows remained unchanged. Therefore the statement that a replacement reads “them” back overstates the evidence; either exercise the acknowledgement/outbox through the real worker or narrow the claim.

Useful? React with 👍 / 👎.

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