fix(ipc): synchronize module status visibility #48

Merged
Breadway merged 2 commits from fix/bread-ipc-status-race into main 2026-10-15 13:37:21 +08:00
Owner

Closes BREAD-48

Root cause

The Lua thread writes M.store.set directly to shared RuntimeState during on_load. If no ModuleStatus exists yet, the old code fabricated a Loaded entry with an empty/partial store and ungated=false. Only after on_load returns does the Lua loader enqueue the actual final status (including ungated=true). A real bread state/modules.list client can therefore observe a loaded module whose store is missing fields and whose status metadata is not final. Separately, the IPC socket is bound before initial Lua loading completes, while ping means only that the socket is accepting connections; the rules tests read health.modules before built-in statuses reach the state engine.

Fix

  • Stage pre-status store writes outside the serialized module list, then merge them into the real status atomically.
  • Add an ordered state-engine barrier and startup.ready, which waits for Lua initial loading and queued status application.
  • Make rules status integration checks await startup readiness without sleeps or weakened assertions.
  • Preserve synchronous reload clears and acknowledge only after clearing state.

Verification

  • Reproduced on unmodified origin/main e4ddf6c under CPU load: 7/8 concurrent full-suite runs failed; the missing store field appeared twice and bread.rules present seven times.
  • Deterministic unit regression covers on_load store write before status and confirms no visible loaded placeholder, then verifies final ungated status and store are merged.
  • Five repeated ipc_integration runs with 24 busy-loop workers: all passed, 43/43 each.
  • cargo test --locked --workspace, cargo clippy --workspace --all-targets --locked -- -D warnings, cargo fmt --all -- --check, and cargo run --locked -p xtask -- check-docs pass.

PR44's module_host_bridge.rs is untouched; this defect is in in-process status publication and startup readiness.

Closes BREAD-48 ## Root cause The Lua thread writes `M.store.set` directly to shared RuntimeState during `on_load`. If no ModuleStatus exists yet, the old code fabricated a `Loaded` entry with an empty/partial store and `ungated=false`. Only after `on_load` returns does the Lua loader enqueue the actual final status (including `ungated=true`). A real `bread state`/`modules.list` client can therefore observe a loaded module whose store is missing fields and whose status metadata is not final. Separately, the IPC socket is bound before initial Lua loading completes, while `ping` means only that the socket is accepting connections; the rules tests read `health.modules` before built-in statuses reach the state engine. ## Fix - Stage pre-status store writes outside the serialized module list, then merge them into the real status atomically. - Add an ordered state-engine barrier and `startup.ready`, which waits for Lua initial loading and queued status application. - Make rules status integration checks await startup readiness without sleeps or weakened assertions. - Preserve synchronous reload clears and acknowledge only after clearing state. ## Verification - Reproduced on unmodified origin/main e4ddf6c under CPU load: 7/8 concurrent full-suite runs failed; the missing store field appeared twice and `bread.rules present` seven times. - Deterministic unit regression covers on_load store write before status and confirms no visible loaded placeholder, then verifies final ungated status and store are merged. - Five repeated `ipc_integration` runs with 24 busy-loop workers: all passed, 43/43 each. - `cargo test --locked --workspace`, `cargo clippy --workspace --all-targets --locked -- -D warnings`, `cargo fmt --all -- --check`, and `cargo run --locked -p xtask -- check-docs` pass. PR44's `module_host_bridge.rs` is untouched; this defect is in in-process status publication and startup readiness.
fix(ipc): synchronize module status visibility
All checks were successful
check / check (push) Successful in 39m43s
a7056d1714
Author
Owner

Independent review finding at a7056d1714: store staging regresses read-your-writes. breadd/src/lua/mod.rs:3130-3144 module_store_get ignores pending_module_stores, so M.store.set("x", 41); assert(M.store.get("x") == 41) in on_load now fails and leaves the module LoadError. An isolated daemon probe against the coordinator built binary reproduced it both at startup and modules.reload; independent exact-head rebuild/checks are still running. Repair on the same PR: reads inside the module must consult its staged values without exposing a Loaded placeholder, with executable Lua startup/reload regression tests. A second concern at mod.rs:333,343,348 in the diff (actual lines 213,237,242): flush_module_statuses errors are discarded and StartupComplete still replies success, contradicting the FIFO acknowledgement guarantee when the state engine is unavailable. Do not merge this head. Full verdict and exact rebuilt evidence will follow; local probe script /tmp/opencode/bread48-review/runtime_probe.py.

Independent review finding at a7056d17148c542019b6818332e538603675925c: store staging regresses read-your-writes. breadd/src/lua/mod.rs:3130-3144 module_store_get ignores pending_module_stores, so M.store.set("x", 41); assert(M.store.get("x") == 41) in on_load now fails and leaves the module LoadError. An isolated daemon probe against the coordinator built binary reproduced it both at startup and modules.reload; independent exact-head rebuild/checks are still running. Repair on the same PR: reads inside the module must consult its staged values without exposing a Loaded placeholder, with executable Lua startup/reload regression tests. A second concern at mod.rs:333,343,348 in the diff (actual lines 213,237,242): flush_module_statuses errors are discarded and StartupComplete still replies success, contradicting the FIFO acknowledgement guarantee when the state engine is unavailable. Do not merge this head. Full verdict and exact rebuilt evidence will follow; local probe script /tmp/opencode/bread48-review/runtime_probe.py.
Author
Owner

Independent review: changes requested

Reviewed SHA: a7056d1714
Base and merge-base: e4ddf6c5d3

Interpretation: hide unfinished in-process module stores/status metadata from production state clients, provide a real Lua completion plus state-engine acknowledgement barrier, preserve working module-store behavior and reload ordering, and publish the IPC contract honestly. Green tests alone do not establish compatibility.

Required repairs on existing PR48:

  • R1, high: breadd/src/lua/mod.rs:3130-3143,3159-3171. module_store_set now stages on_load writes, but module_store_get ignores that staging. A real legacy module doing M.store.set("x",41); assert(M.store.get("x")==41) fails to load. Independently built exact head reproduces load_error at startup AND modules.reload, with x=41 ultimately visible and y absent. Independently built old base e4ddf6c executes the same module successfully, with x=41 and y=42. Read private staged values from inside the module without exposing a Loaded placeholder. Add executable Lua startup/reload regressions covering read-after-write and repeated writes, not only direct Rust map assertions. Confidence high, deterministic actual daemon evidence. This is a new regression introduced by the BREAD-48 staging change and is recorded against this existing repair card rather than a duplicate.
  • R2: breadd/src/lua/mod.rs:112-113,147-155,207-214,234-243. flush_module_statuses returns an error if the command channel closes or acknowledgement is dropped, but every call discards it. StartupComplete sends () regardless, so startup.ready can report ok without the promised state-engine acknowledgement; successful reload likewise discards the barrier error. Carry the barrier Result back to IPC and propagate errors. Add tests for closed state-engine channel and dropped acknowledgement, plus successful ordered acknowledgement. Define/document whether startup.ready means initial load finished attempting (even if init failed), rather than healthy modules; errors in init currently only log. Module LoadError may legitimately be an inspected result, but engine acknowledgement failure is not success. Confidence high from explicit error-discarding branches; failure-path runtime injection was not performed.

Criterion mapping:

  1. No fabricated Loaded/ungated placeholder: pass for the targeted publication path. pending_module_stores is serde-skipped (core/types.rs:18-23); SetModuleStatus merges under one write lock (core/state_engine.rs:368-390). Exact-head store_write_during_on_load_is_not_visible_as_loaded_until_status_arrives passes. Old source unconditionally pushes Loaded in module_store_set; the old/new executable compatibility probe above exposes the missing regression coverage in the new test.
  2. Module store compatibility: fail, R1 independently reproduced, old works/new fails.
  3. Initial Lua completion plus FIFO state acknowledgement: normal path works, error contract fails R2. IPC routes through Lua queue and state-engine Barrier; five stress suites pass. ping remains connectivity-only and health/state may show incomplete startup until clients explicitly await startup.ready. That is acceptable only as an explicit completion API contract, not a claim all existing production health clients are now gated. Nothing in this PR gates health or changes existing clients.
  4. Reload clear ordering: source and unit check pass. Synchronous clear acknowledgements and clear_modules_blocks_until_the_engine_has_applied_it are included in the actual PR diff, not present in base. Inspected the existing dirty bread-ipc-ungated-readiness state_engine diff: it contains these earlier acknowledgements/test; PR48 preserves them, adds staging cleanup/barrier, and does not bundle PR44 module_host_bridge work. The primary checkout and that preexisting dirty worktree were not edited.
  5. Public schema/docs: registry and check-docs pass (Documentation.md:1759, api-schema.toml startup.ready entry). Readiness failure/attempt semantics need the clarification in R2.
  6. Full checks and stress: pass for current checked-in coverage, but insufficient for R1/R2.

Independent checks on exact head:

  • cargo fmt --all -- --check: pass.
  • cargo run --locked -p xtask -- check-docs: pass.
  • cargo build --workspace --locked: pass in initial independent run.
  • cargo clippy --workspace --all-targets --locked -- -D warnings: pass.
  • cargo test --workspace --locked: pass, including breadd 200 unit tests and 43 IPC tests.
  • Five fresh ipc_integration binaries --test-threads=8 with 24 concurrent CPU workers: 43/43 each, 2.02-2.53 seconds. Workers always cleaned up. An earlier pre-interruption attempt timed out on run three at 240 seconds after two successes; temporary logs were lost across the interrupted session, so no cause is assigned and it is not represented as five successes. Fresh durable logs retained.
  • python3 probe.py : R1 LoadError at startup and reload.
  • cargo build --locked -p breadd --manifest-path /Cargo.toml, then identical probe: loaded, x=41/y=42 at startup and reload. Shared scratch target was reused only after exact-head checks and stress finished; target/debug/breadd now contains the old-base executable. Rebuild exact head before using that path again.

CI: check / check (push) SUCCESS on exact SHA a7056d1714; Actions run152, internal2521, check.yml status success, 39m43s. No branch protections. check.yml lint/build/test is the applicable branch gate; main/tag publishing workflows are not PR-head gates. Existing formal reviews API is empty. All PR and changed-file list pages exhausted, seven actual files read. PR48 uniquely closes BREAD-48. No issue dependencies from Forgejo; BREAD-28 is the downstream verification card, not a prerequisite.

Verdict changes_requested despite green CI. No merge, tags, publish or done. Preserve PR/branch/worktrees. The requested review placement conflicts with the required changes-requested action: move BREAD-48 to todo for coordinator rework, not done. Leave BREAD-28 verification untouched. Fresh review must target the repaired SHA with green CI. Durable evidence: ~/.config/breadboard/reviews/BREAD-48-a7056d1/. No live desktop or remote-machine access.

## Independent review: changes requested Reviewed SHA: a7056d17148c542019b6818332e538603675925c Base and merge-base: e4ddf6c5d38e43e5cbddd8093e4ca042810371b5 Interpretation: hide unfinished in-process module stores/status metadata from production state clients, provide a real Lua completion plus state-engine acknowledgement barrier, preserve working module-store behavior and reload ordering, and publish the IPC contract honestly. Green tests alone do not establish compatibility. Required repairs on existing PR48: - [ ] R1, high: breadd/src/lua/mod.rs:3130-3143,3159-3171. module_store_set now stages on_load writes, but module_store_get ignores that staging. A real legacy module doing M.store.set("x",41); assert(M.store.get("x")==41) fails to load. Independently built exact head reproduces load_error at startup AND modules.reload, with x=41 ultimately visible and y absent. Independently built old base e4ddf6c executes the same module successfully, with x=41 and y=42. Read private staged values from inside the module without exposing a Loaded placeholder. Add executable Lua startup/reload regressions covering read-after-write and repeated writes, not only direct Rust map assertions. Confidence high, deterministic actual daemon evidence. This is a new regression introduced by the BREAD-48 staging change and is recorded against this existing repair card rather than a duplicate. - [ ] R2: breadd/src/lua/mod.rs:112-113,147-155,207-214,234-243. flush_module_statuses returns an error if the command channel closes or acknowledgement is dropped, but every call discards it. StartupComplete sends () regardless, so startup.ready can report ok without the promised state-engine acknowledgement; successful reload likewise discards the barrier error. Carry the barrier Result back to IPC and propagate errors. Add tests for closed state-engine channel and dropped acknowledgement, plus successful ordered acknowledgement. Define/document whether startup.ready means initial load finished attempting (even if init failed), rather than healthy modules; errors in init currently only log. Module LoadError may legitimately be an inspected result, but engine acknowledgement failure is not success. Confidence high from explicit error-discarding branches; failure-path runtime injection was not performed. Criterion mapping: 1. No fabricated Loaded/ungated placeholder: pass for the targeted publication path. pending_module_stores is serde-skipped (core/types.rs:18-23); SetModuleStatus merges under one write lock (core/state_engine.rs:368-390). Exact-head store_write_during_on_load_is_not_visible_as_loaded_until_status_arrives passes. Old source unconditionally pushes Loaded in module_store_set; the old/new executable compatibility probe above exposes the missing regression coverage in the new test. 2. Module store compatibility: fail, R1 independently reproduced, old works/new fails. 3. Initial Lua completion plus FIFO state acknowledgement: normal path works, error contract fails R2. IPC routes through Lua queue and state-engine Barrier; five stress suites pass. ping remains connectivity-only and health/state may show incomplete startup until clients explicitly await startup.ready. That is acceptable only as an explicit completion API contract, not a claim all existing production health clients are now gated. Nothing in this PR gates health or changes existing clients. 4. Reload clear ordering: source and unit check pass. Synchronous clear acknowledgements and clear_modules_blocks_until_the_engine_has_applied_it are included in the actual PR diff, not present in base. Inspected the existing dirty bread-ipc-ungated-readiness state_engine diff: it contains these earlier acknowledgements/test; PR48 preserves them, adds staging cleanup/barrier, and does not bundle PR44 module_host_bridge work. The primary checkout and that preexisting dirty worktree were not edited. 5. Public schema/docs: registry and check-docs pass (Documentation.md:1759, api-schema.toml startup.ready entry). Readiness failure/attempt semantics need the clarification in R2. 6. Full checks and stress: pass for current checked-in coverage, but insufficient for R1/R2. Independent checks on exact head: - cargo fmt --all -- --check: pass. - cargo run --locked -p xtask -- check-docs: pass. - cargo build --workspace --locked: pass in initial independent run. - cargo clippy --workspace --all-targets --locked -- -D warnings: pass. - cargo test --workspace --locked: pass, including breadd 200 unit tests and 43 IPC tests. - Five fresh ipc_integration binaries --test-threads=8 with 24 concurrent CPU workers: 43/43 each, 2.02-2.53 seconds. Workers always cleaned up. An earlier pre-interruption attempt timed out on run three at 240 seconds after two successes; temporary logs were lost across the interrupted session, so no cause is assigned and it is not represented as five successes. Fresh durable logs retained. - python3 probe.py <independently built exact-head breadd>: R1 LoadError at startup and reload. - cargo build --locked -p breadd --manifest-path <isolated old base>/Cargo.toml, then identical probe: loaded, x=41/y=42 at startup and reload. Shared scratch target was reused only after exact-head checks and stress finished; target/debug/breadd now contains the old-base executable. Rebuild exact head before using that path again. CI: check / check (push) SUCCESS on exact SHA a7056d17148c542019b6818332e538603675925c; Actions run152, internal2521, check.yml status success, 39m43s. No branch protections. check.yml lint/build/test is the applicable branch gate; main/tag publishing workflows are not PR-head gates. Existing formal reviews API is empty. All PR and changed-file list pages exhausted, seven actual files read. PR48 uniquely closes BREAD-48. No issue dependencies from Forgejo; BREAD-28 is the downstream verification card, not a prerequisite. Verdict changes_requested despite green CI. No merge, tags, publish or done. Preserve PR/branch/worktrees. The requested review placement conflicts with the required changes-requested action: move BREAD-48 to todo for coordinator rework, not done. Leave BREAD-28 verification untouched. Fresh review must target the repaired SHA with green CI. Durable evidence: ~/.config/breadboard/reviews/BREAD-48-a7056d1/. No live desktop or remote-machine access.
fix(ipc): read staged stores during load, propagate barrier errors
All checks were successful
check / check (push) Successful in 17m24s
f37efff325
Author
Owner

Repair for review 4989 (R1, R2), pushed as f37efff on this branch.

R1: module_store_get now reads the private staged store first, then the finalized store. Staged keys shadow finalized ones, replacement and per-module isolation follow from the staged map. Added in_process_module_store_supports_read_your_writes_on_startup_and_reload: a real daemon runs a legacy module that asserts read-your-writes in on_load and rewrites keys, verified at startup and after modules.reload. Confirmed it fails on the reverted overlay (module load_error) and passes with the repair. No Loaded placeholder is exposed: staging stays out of serialized state and merges atomically with final status.

R2: the initial, reload, and StartupComplete paths now return state-engine barrier failures instead of discarding them. startup.ready and modules.reload surface those errors over IPC; the Lua channel send failure and dropped acknowledgement keep their distinct errors. Documented in Documentation.md: readiness means initial loading completed plus acknowledged state application. Module load errors stay visible in module status and do not fail readiness. Added flush_module_statuses_reports_closed_channel_and_dropped_ack and flush_module_statuses_acknowledges_only_after_prior_commands.

Checks on f37efff: cargo test --locked --workspace pass, clippy -D warnings clean, fmt clean, check-docs OK. IPC suite 44/44 in five loaded runs. Note: under extreme oversubscription (16 to 24 busy loops plus another session's heavy workload on the same box) the pre-existing events_stream_receives_emitted_events stream read blocked twice; that test has no socket timeout and the event path is untouched by this diff. Same signature was seen on unmodified code in earlier baseline runs.

Repair for review 4989 (R1, R2), pushed as f37efff on this branch. R1: `module_store_get` now reads the private staged store first, then the finalized store. Staged keys shadow finalized ones, replacement and per-module isolation follow from the staged map. Added `in_process_module_store_supports_read_your_writes_on_startup_and_reload`: a real daemon runs a legacy module that asserts read-your-writes in `on_load` and rewrites keys, verified at startup and after `modules.reload`. Confirmed it fails on the reverted overlay (module load_error) and passes with the repair. No `Loaded` placeholder is exposed: staging stays out of serialized state and merges atomically with final status. R2: the initial, reload, and `StartupComplete` paths now return state-engine barrier failures instead of discarding them. `startup.ready` and `modules.reload` surface those errors over IPC; the Lua channel send failure and dropped acknowledgement keep their distinct errors. Documented in `Documentation.md`: readiness means initial loading completed plus acknowledged state application. Module load errors stay visible in module status and do not fail readiness. Added `flush_module_statuses_reports_closed_channel_and_dropped_ack` and `flush_module_statuses_acknowledges_only_after_prior_commands`. Checks on f37efff: `cargo test --locked --workspace` pass, clippy `-D warnings` clean, fmt clean, check-docs OK. IPC suite 44/44 in five loaded runs. Note: under extreme oversubscription (16 to 24 busy loops plus another session's heavy workload on the same box) the pre-existing `events_stream_receives_emitted_events` stream read blocked twice; that test has no socket timeout and the event path is untouched by this diff. Same signature was seen on unmodified code in earlier baseline runs.
Author
Owner

Independent re-review: PASS for exact PR head f37efff325. Reviewed full BREAD-48 diff and verified merged tree d7add44b20 has the exact same Git tree (4b1b909491); merge parents are e4ddf6c and f37efff.

Criterion mapping: (1) pending module stores remain serde-skipped and merge atomically into final status; status staging unit passes. (2) read-your-writes and repeated staged writes now use pending store overlay; actual daemon probe passes at startup and modules.reload with final x=42, other=isolated, status=loaded. (3) state-engine barrier propagates closed-channel/dropped-ack errors and acknowledges FIFO prior status; tests cover both failure cases and ordered success. Lua startup readiness carries initialization/barrier errors, documents module load errors as status rather than readiness failure, retains ping connectivity-only. (4) synchronous clear/module-widget acknowledgements preserved and tested. (5) startup.ready schema and documentation contract check pass.

Independent evidence, scratch ~/.worktrees/bread/fix/review-merged-pr48-f37efff and ~/.config/breadboard/reviews/BREAD-48-f37efff/: cargo fmt --all -- --check PASS; cargo run --locked -p xtask -- check-docs PASS; cargo build --workspace --locked PASS; cargo clippy --workspace --all-targets --locked -- -D warnings PASS; cargo test --locked --workspace PASS (includes breadd 202 unit, IPC 44 and all workspace/doc test targets). Fresh actual daemon read/write/startup/reload probe passes against f37efff. Exact-head CI run154/check (push) succeeded at f37efff. Integrated main run156/dev release build succeeded on d7add44. Fresh loaded stress with 24 busy workers yielded four 44/44 runs; one run had unrelated profile_activate_updates_state failure (state dump observed default after activating battery), and the targeted test passes alone immediately afterward. Separately observed prior CI run152 at ancestor SHA a7056d1, not attributed to f37efff. The exact-head CI, integrated main workflow and independent locked suite are green; saturation-only unrelated profile timing flake is recorded.

No formal review entries or branch protections; exact head CI and integrated status individually verified green. No merge performed by reviewer. Historical PR was merged before this review; integration is now independently confirmed. Keep BREAD-48 in review pending coordinator verification, do not mark done. BREAD-28 verification remains coordinator-owned.

Independent re-review: PASS for exact PR head f37efff32516294f41112b0e74fa142d60196599. Reviewed full BREAD-48 diff and verified merged tree d7add44b204b7ce4a3648d5e98f32475ee705512 has the exact same Git tree (4b1b909491f2376dd9bd6c951a844548a9bfe8db); merge parents are e4ddf6c and f37efff. Criterion mapping: (1) pending module stores remain serde-skipped and merge atomically into final status; status staging unit passes. (2) read-your-writes and repeated staged writes now use pending store overlay; actual daemon probe passes at startup and modules.reload with final x=42, other=isolated, status=loaded. (3) state-engine barrier propagates closed-channel/dropped-ack errors and acknowledges FIFO prior status; tests cover both failure cases and ordered success. Lua startup readiness carries initialization/barrier errors, documents module load errors as status rather than readiness failure, retains ping connectivity-only. (4) synchronous clear/module-widget acknowledgements preserved and tested. (5) startup.ready schema and documentation contract check pass. Independent evidence, scratch ~/.worktrees/bread/fix/review-merged-pr48-f37efff and ~/.config/breadboard/reviews/BREAD-48-f37efff/: cargo fmt --all -- --check PASS; cargo run --locked -p xtask -- check-docs PASS; cargo build --workspace --locked PASS; cargo clippy --workspace --all-targets --locked -- -D warnings PASS; cargo test --locked --workspace PASS (includes breadd 202 unit, IPC 44 and all workspace/doc test targets). Fresh actual daemon read/write/startup/reload probe passes against f37efff. Exact-head CI run154/check (push) succeeded at f37efff. Integrated main run156/dev release build succeeded on d7add44. Fresh loaded stress with 24 busy workers yielded four 44/44 runs; one run had unrelated profile_activate_updates_state failure (state dump observed default after activating battery), and the targeted test passes alone immediately afterward. Separately observed prior CI run152 at ancestor SHA a7056d1, not attributed to f37efff. The exact-head CI, integrated main workflow and independent locked suite are green; saturation-only unrelated profile timing flake is recorded. No formal review entries or branch protections; exact head CI and integrated status individually verified green. No merge performed by reviewer. Historical PR was merged before this review; integration is now independently confirmed. Keep BREAD-48 in review pending coordinator verification, do not mark done. BREAD-28 verification remains coordinator-owned.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Breadway/bread!48
No description provided.