fix(ipc): synchronize module status visibility #48
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/bread-ipc-status-race"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes BREAD-48
Root cause
The Lua thread writes
M.store.setdirectly to shared RuntimeState duringon_load. If no ModuleStatus exists yet, the old code fabricated aLoadedentry with an empty/partial store andungated=false. Only afteron_loadreturns does the Lua loader enqueue the actual final status (includingungated=true). A realbread state/modules.listclient 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, whilepingmeans only that the socket is accepting connections; the rules tests readhealth.modulesbefore built-in statuses reach the state engine.Fix
startup.ready, which waits for Lua initial loading and queued status application.Verification
e4ddf6cunder CPU load: 7/8 concurrent full-suite runs failed; the missing store field appeared twice andbread.rules presentseven times.ipc_integrationruns 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, andcargo run --locked -p xtask -- check-docspass.PR44's
module_host_bridge.rsis untouched; this defect is in in-process status publication and startup readiness.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: changes requested
Reviewed SHA:
a7056d1714Base and merge-base:
e4ddf6c5d3Interpretation: 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:
e4ddf6cexecutes 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.Criterion mapping:
Independent checks on exact head:
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.
Repair for review 4989 (R1, R2), pushed as
f37efffon this branch.R1:
module_store_getnow 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. Addedin_process_module_store_supports_read_your_writes_on_startup_and_reload: a real daemon runs a legacy module that asserts read-your-writes inon_loadand rewrites keys, verified at startup and aftermodules.reload. Confirmed it fails on the reverted overlay (module load_error) and passes with the repair. NoLoadedplaceholder is exposed: staging stays out of serialized state and merges atomically with final status.R2: the initial, reload, and
StartupCompletepaths now return state-engine barrier failures instead of discarding them.startup.readyandmodules.reloadsurface those errors over IPC; the Lua channel send failure and dropped acknowledgement keep their distinct errors. Documented inDocumentation.md: readiness means initial loading completed plus acknowledged state application. Module load errors stay visible in module status and do not fail readiness. Addedflush_module_statuses_reports_closed_channel_and_dropped_ackandflush_module_statuses_acknowledges_only_after_prior_commands.Checks on
f37efff:cargo test --locked --workspacepass, clippy-D warningsclean, 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-existingevents_stream_receives_emitted_eventsstream 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.Independent re-review: PASS for exact PR head
f37efff325. Reviewed full BREAD-48 diff and verified merged treed7add44b20has the exact same Git tree (4b1b909491); merge parents aree4ddf6candf37efff.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 atf37efff. Integrated main run156/dev release build succeeded ond7add44. 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 SHAa7056d1, not attributed tof37efff. 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.