[issue-3077][slice-6/6] Bounded durability: fail-loud... - #3144
Conversation
…ore fallback When EGG_MESSAGE_STORE_BACKEND is unset/"auto" and Redis is unreachable, the backend selection silently fell back to the in-memory store, leaving the orchestrator vulnerable to the #3076 mid-phase-restart loss risk operators didn't opt into. This slice surfaces that risk two ways: - An error-level structured log with the stable marker token MESSAGE_STORE_AUTO_FALLBACK_TO_MEMORY (scrapers/alerts can pin on a single string rather than prose). - A module-level degraded flag exposed via is_memory_fallback_degraded() and surfaced on /api/v1/health under components.message_store, flipping the top-level status to "degraded". Explicit EGG_MESSAGE_STORE_BACKEND=memory (dev/test intent) emits at warning level and does NOT set the degraded flag — the operator opted in. Per HITL Q3: warn/error + health flag only — auto selection semantics and no-refusal-to-run are unchanged. Both log emissions are once-per-process (reset_message_store does not clear the once-flags) so integration-test harnesses that reset the singleton many times per pytest run aren't spammed; tests for this feature use _reset_memory_fallback_state_for_test to re-arm. The health-route read goes through the message_store module's pure getter, preserving the issue #1897 TASK-4-3 isolation invariant — no MessageStore method is called on the /health request path. Added a regression test that re-locks that invariant for the slice-6 surface. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
TASK-6-2: extend message-store test coverage for the bounded-durability slice (HITL Q3 fail-loud only, no refusal). orchestrator/tests/test_message_store.py: - TestBackendSelectionFailLoudMatrix pins TASK-6-1's contract on _create_message_store(): auto→memory fallback emits a single ERROR-level log with a stable, grep-able marker (MEMORY_FALLBACK_MARKER) and flips a module-level degraded flag (is_message_store_degraded()) the health surface attaches to; explicit memory backend stays at WARNING and keeps the flag clear; Redis-resolved auto is silent; explicit redis with Redis down still raises. Two sequencing pins: a subsequent Redis-success _create_message_store() clears the flag (boot-time blip recovers), and an explicit memory selection after an auto-fallback clears the flag (operator intent erases the degradation). orchestrator/tests/test_redis_message_store.py: - TestRedisRestartSemanticsVsPhaseBoundaryWipe asserts the two durability semantics together so #3076 mid-phase loss (defect) and _clear_concurrent_state phase-boundary wipe (required behavior) cannot be conflated. Restart pins: transcript bytes, per-type counters, and since_id scan-fallback resolution all survive a re-instantiated RedisMessageStore against the same Redis backend. Wipe pin: _clear_concurrent_state() routed at the singleton drains the Redis stream as designed. Combined invariant: a phase-boundary wipe followed by a restart stays empty (the wipe is persistent). The fail-loud matrix tests intentionally fail until TASK-6-1 lands the MEMORY_FALLBACK_MARKER constant and is_message_store_degraded() query function — this is the parallel-root contract pin, not a regression. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…moryFallbackHardening Pulls TASK-6-1 (coder) on top of the tester branch and resolves the test_message_store.py overlap by keeping the coder's TestMemoryFallbackFailLoudSignal class (covers the basic matrix: auto-fallback → ERROR + degraded; explicit memory → WARNING; redis silent; once-per-process; reset helper rearms) and adding a new tester-side TestMemoryFallbackHardening class with operational pins the coder's tests don't make explicit: - test_degraded_flag_sticky_across_redis_recovery: pins that a subsequent successful Redis selection MUST NOT clear the degraded flag — the operator-facing /api/v1/health signal stays set until restart / test-reset so the brief durability window isn't hidden. - test_warn_and_error_once_flags_are_independent (both orderings): the WARNING (explicit memory) and ERROR (auto fallback) once-flags are distinct; a process exercising both paths emits exactly one of each regardless of which fired first, and explicit memory after an auto-fallback does NOT clear the degraded flag. - test_concurrent_auto_fallback_emits_marker_at_most_once: four-thread barrier race on _create_message_store() asserts at-most-one marker log and a monotonic-True degraded flag — would catch a future refactor that drops the check-then-set atomicity. Also keeps the tester's TestRedisRestartSemanticsVsPhaseBoundaryWipe class from the prior commit (5 tests pinning that a re-instantiated RedisMessageStore against the same fakeredis preserves transcript / counters / since_id resolution, while _clear_concurrent_state still wipes at the phase boundary — the two semantics asserted side-by-side so the designed wipe cannot be misread as accidental mid-phase loss). 123 tests pass under `.venv/bin/pytest orchestrator/tests/test_message_store.py orchestrator/tests/test_redis_message_store.py`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ruff format wanted to collapse a 3-line def into one line (test_health_endpoint_message_store_surface_does_not_call_messagestore). The coder's slice-6 commit added it as multiline, but the project's formatter prefers the single-line form for this width. No test logic changes; this satisfies `make lint` so the tester producer's re-propose passes the lint pre-check.
e6872e4 to
cee49a4
Compare
This comment has been minimized.
This comment has been minimized.
Slice-6 added a once-per-process message_store._memory_fallback_degraded global that /api/v1/health surfaces and uses to degrade top-level status. In CI (no Redis), auto backend selection trips it, polluting sibling health tests that assert 'healthy'. Add autouse fixtures resetting the flag, mirroring test_health_routes.py.
Autofix tracking{"Test/Unit Tests": 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… conflict in .egg-state/contracts/issue-3077.json Took the base branch's progressed pipeline state (slices/tasks marked complete with commit SHAs and PR numbers, additive cq-3 decision and audit-log entries). HEAD only carried older pending/null placeholders.
Conflict Resolution SummaryResolved merge conflicts with
Details — the only conflicting file was the generated SDLC contract state file. All 8 conflict hunks were the base branch (
A No application code conflicted — slice-6's actual code ( Verification: Please review: Nothing security- or logic-sensitive — the resolution is confined to generated pipeline bookkeeping state, taking the integration branch's up-to-date values. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contract Verification — PR #3144 (issue-3077 slice-6/6)
Verdict: Approve. Both contract tasks are fully implemented and all acceptance criteria are objectively met. I ran the new test classes directly (18 tests, all green) and traced each criterion to its implementing code.
Note: the
issue-3077contract carries acceptance criteria as per-task free text (no top-levelac-Nids), and the orchestrator CLI was unreachable this session, so there are noverify-criterion --criterion ac-Nmarkers to set. Verification is recorded here instead.
task-6-1 — Fail-loud memory-backend signal (message_store.py) ✅
| Criterion | Evidence | Status |
|---|---|---|
auto→memory: exactly one error log w/ stable marker + health degraded field |
_create_message_store() except-branch sets _memory_fallback_degraded=True and emits logger.error("%s: …", MEMORY_FALLBACK_MARKER, extra={"marker":…,"error":…}) guarded by _memory_fallback_logged (message_store.py:714-741); health.py:148-165 reads is_memory_fallback_degraded() into components.message_store and degrades top-level status |
Verified |
Explicit memory: warning level, no flag |
message_store.py:689-700 — logger.warning(... explicit EGG_MESSAGE_STORE_BACKEND=memory ...) once-per-process, returns MessageStore(), flag untouched |
Verified |
| Redis backend: neither | success path logs info only; use_redis=="redis" failure re-raises (fail-hard) — no flag, no warn/error |
Verified |
auto selection unchanged; tests not spammed |
Selection precedence identical to #1897 design; _memory_fallback_logged/_memory_explicit_logged are once-per-process and reset_message_store() deliberately does NOT clear them; _reset_memory_fallback_state_for_test() re-arms for tests |
Verified |
Health-surface isolation invariant (#1897 TASK-4-3) is preserved: health.py imports the message_store module and reads the pure getter is_memory_fallback_degraded() — no MessageStore method is touched on the request path. Locked by test_health_endpoint_message_store_surface_does_not_call_messagestore.
task-6-2 — Durability tests ✅
| Criterion | Evidence | Status |
|---|---|---|
| Redis mid-phase restart preserves transcript | TestRedisRestartSemanticsVsPhaseBoundaryWipe::test_mid_phase_restart_preserves_transcript_via_shared_redis (+ counter/since_id variants) — new RedisMessageStore against same Redis observes all prior messages by id |
Verified |
_clear_concurrent_state() still wipes at phase boundary |
test_phase_boundary_clear_concurrent_state_still_wipes invokes routes.phases._clear_concurrent_state (confirmed at phases.py:113) and asserts the stream drains |
Verified |
| Fail-loud matrix asserted (error/warning/silent × flag) | TestMemoryFallbackFailLoudSignal covers all four rows; TestMemoryFallbackHardening adds sticky-flag, independent-once-flags, and concurrency pins |
Verified |
| Both wipe semantics named explicitly | test_mid_phase_restart_* vs test_phase_boundary_clear_* ids + class docstring distinguish accidental loss from designed wipe |
Verified |
Test run (targeted; full suite intentionally not run)
tests/test_message_store.py::TestMemoryFallbackFailLoudSignal ...... [6 passed]
tests/test_message_store.py::TestMemoryFallbackHardening .... [4 passed]
tests/test_health_routes.py::TestMessageStoreFailLoudSurface ... [3 passed]
tests/test_redis_message_store.py::TestRedisRestartSemanticsVsPhaseBoundaryWipe ..... [5 passed]
= 18 passed =
Non-blocking observations
task-6-1.files_affectedis under-inclusive. It lists onlyorchestrator/message_store.py, but the PR also editsorchestrator/routes/health.py(required to satisfy the "visible on the health surface" criterion) and addsautousereset fixtures totest_health_check_integration.py,test_health_check_lifecycle_integration.py, andtest_state_store_wedge_propagation.py. These are all legitimate and in-scope — the health wiring is mandatory for the AC, and the sibling-test fixtures prevent the new process-global from flaking order-dependent tests in CI (where Redis is absent andautofalls back). Not orphaned code; worth noting only for contract bookkeeping.- Concurrency test asserts
<= 1rather than== 1.test_concurrent_auto_fallback_emits_marker_at_most_oncehonestly documents that the once-flag uses unsynchronized module globals (no explicit lock), so a check-then-set race could in principle emit zero under reset interplay. This is a conservative, correctly-reasoned pin for the bounded-durability scope, not a defect — the monotonic degraded-flag half is asserted unconditionally. - The two
.egg-state/brc-history/3077-implement-slice-6.{json,md}files (~17.5k of the 18.6k added lines) are BRC consensus artifacts, not code — expected for this pipeline.
No contract violations, no correctness issues, no missing tests. Selection semantics and the #1897 isolation invariant are preserved exactly as the HITL-Q3 constraint requires.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: slice-6 fail-loud auto→memory message-store fallback
I traced the full data flow (production trigger → flag → health surface) and reviewed every changed code file against the production code paths. No blocking issues. The feature works end-to-end and the tests exercise real production code, not hand-built fixtures or self-seeding goldens. Details and three non-blocking notes below.
End-to-end functionality: verified
The core question for this slice is whether the auto→memory fallback actually trips the health surface in the real runtime. It does:
redis_message_store.get_redis_message_store()eagerlyclient.ping()s and raisesConnectionErroron a down Redis (redis_message_store.py:589-592), so_create_message_store()'sautobranch genuinely reaches theexceptand sets_memory_fallback_degraded = True— this is not a lazy connection that would defer the failure past store creation.routes/health.py:137reads the flag off the imported module (is_memory_fallback_degraded()), a pure global read — the issue #1897 TASK-4-3 isolation invariant (noMessageStore.*calls on the request path) is preserved, andtest_health_endpoint_message_store_surface_does_not_call_messagestorelocks it.- health.py and the dominant store-creation path both import the flat
message_storemodule, so the flag is set and read through the same module object./api/v1/healthstill returns HTTP 200; only the JSONstatusfield flips.
Tests are well-constructed: TestRedisRestartSemanticsVsPhaseBoundaryWipe drives real RedisMessageStore.add_message/get_messages/get_status/clear over a shared fakeredis instance to model "same Redis, new store object," and correctly pins the intentional phase-boundary wipe (_clear_concurrent_state) separately from the accidental-loss failure class. The fail-loud matrix (error/warning/silent × flag) maps cleanly to the acceptance criteria.
Non-blocking notes
1. Blast radius of the top-level status flip. healthy = state_store_healthy and not message_store_degraded now drives both the top-level status: "degraded" and _health_tracker.record(healthy). Because the degraded flag is sticky by design (only restart/test-reset clears it), any deployment running on the memory fallback reports status: "degraded" permanently and the tracker records a permanent unhealthy transition (healthy_since → null for the process lifetime). This is the intended HITL-Q3 behavior, and HTTP stays 200 so LB probes keying on status code are unaffected — but please confirm mcp__egg__check_health, dashboards, and any alerting that parses the status field treat "degraded" as informational rather than a paging/hard-down condition. /ready is correctly left untouched (state-store-only), so the pod isn't pulled from rotation.
2. Dual-import hazard for the module-global signal (pre-existing). Almost the entire codebase imports from message_store import ... (flat), but routes/pipelines.py:17394 imports from orchestrator.message_store import .... Since orchestrator/ is a package, those resolve to two distinct module objects with independent globals — including the new _memory_fallback_degraded flag and the _message_store singleton. In practice the fallback is first tripped via a flat-import path at startup (e.g. concurrent_executor.py:39 imports get_message_store at module load), so health (flat) observes it correctly and this is not a silent no-op for this feature. But the new signal now adds reliance on cross-module module-global state, which makes that lone orchestrator.message_store import a latent reliability/split-brain seam worth normalizing to the flat form (separately, it implies a second message-store singleton on that path).
3. Once-flag check-then-set is unsynchronized — but safe in production. _create_message_store() mutates _memory_fallback_logged / _memory_explicit_logged without a lock. Production only reaches it via get_message_store() under _store_lock (double-checked locking), so it's effectively serialized; the direct _create_message_store() calls in the concurrency test bypass that lock, which is why the test conservatively asserts <= 1. The degraded-flag set is monotonic, so the worst case is a duplicate log line, never a missed degradation. Fine as-is; a one-line note that _store_lock is the real guard would help future readers.
Scope confirmation
.egg-state/brc-history/*artifacts skipped per review conventions.- Selection precedence unchanged (HITL Q3 honored): explicit
redisstill fails hard (test_explicit_redis_raises_on_failure_unchanged); explicitmemorywarns without the flag;autofallback errors + flags. - Flakiness mitigation is complete: the three test classes that hit the real
/api/v1/healthand assert top-levelhealthy(test_health_check_integration.py,test_health_check_lifecycle_integration.py,test_state_store_wedge_propagation.py) all received the autouse reset fixture; the otherhealth-asserting tests (test_cli.py,test_gateway_client.py,test_state_store_probe.py) use mocks or probesnapshot()directly and are unaffected.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…guard - Normalize the lone orchestrator.message_store package import in routes/pipelines.py to the flat 'from message_store import ...' form used everywhere else, so the slice-6 module-global fallback signal (and the singleton) live on a single module object (egg-reviewer note 2). - Add a comment in _create_message_store clarifying that _store_lock is the real serialization guard for the unsynchronized once-flags, so the worst case for a direct caller is a duplicate log, never a missed degradation (egg-reviewer note 3).
Review feedback addressedBoth reviews approved (contract-verification + slice-6 review). Per-item disposition of the non-blocking notes below. egg-reviewer[bot]
james-in-a-box[bot] (contract verification)
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: slice-6 fail-loud auto→memory fallback (delta 725b51e → 581ec33)
I re-fetched the full history and reviewed the PR-authored delta against the merge base. The delta is exactly two changes, both addressing my prior non-blocking notes. No blocking issues. CI is fully green (Unit + Integration Tests, Python, Security Scan all SUCCESS).
Note 2 (dual-import / split-brain singleton) — fixed correctly
routes/pipelines.py:17394 now imports the flat from message_store import Message, get_message_store instead of the package form from orchestrator.message_store import …. I verified this is the right fix, not a cosmetic one:
- The flat form genuinely resolves in this runtime —
pipelines.py's own top-level imports are flat (from lifecycle_auth import …at :313,from routes import …at :1153), so if flat resolution were broken the module wouldn't import at all.health.py:41(import message_store as _message_store_module) andconcurrent_executor.py:39also use the flat form. - This unifies the singleton: the OVERSEER_ALERT emitted on the cascade path now lands in the same module-global
_message_storebuffer that the messages route reads, and shares the same_memory_fallback_degradedflag thathealth.py:137reads. The old package-form import would have created a second module object with an independent store and flag — the exact split-brain seam I flagged. - Grep confirms zero remaining
from orchestrator.message_store importstatements; the only remaining"orchestrator.message_store"strings are logger-name references in tests, which are correct.
Note 3 (unsynchronized once-flag) — documented correctly
The new comment in _create_message_store() (message_store.py:686-692) accurately reflects the code: get_message_store() (:656-667) serializes creation via _store_lock double-checked locking, and the degraded-flag set (:730) is monotonic within a process (only the test-reset helper at :649-651 clears it). Worst case for a direct unsynchronized caller is a duplicate log line, never a missed degradation. Accurate.
Non-blocking
- Import-form consistency. Every other
message_storeimport site inpipelines.pyuses the dual-formtry: from message_store … except ImportError: from ..message_store …(e.g. :1985-1987, :4386-4389, :15392-15394). The changed line at :17394 uses the bare flat form with no..message_storefallback. Because it's wrapped inexcept Exception: # noqa: BLE001 … pass, an import failure degrades gracefully to the always-on log line (no crash), and the prior code had no fallback either — so this is not a regression. But adopting the file's established dual-form for consistency would make the cascade-alert path robust under the package-import contexts (e.g. some unit tests) where only the relative form resolves. Optional.
Prior Note 1 (sticky status: "degraded" blast radius) was reasonably defended as intended HITL-Q3 behavior, with HTTP staying 200 and /ready untouched — no code change warranted there.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract re-verification — slice-6 (#3077), delta since 725b51e
Re-reviewed the incremental delta on top of my prior verification at 725b51e. The PR branch was rebased (shallow clone; 725b51e is no longer locally reachable), but the GitHub commit timeline confirms the delta is exactly one commit — 581ec33 "Address review: normalize message_store import, document _store_lock guard" — touching two files:
| File | Change | Review note |
|---|---|---|
orchestrator/message_store.py |
+7 lines, pure comment in _create_message_store |
note 3 |
orchestrator/routes/pipelines.py |
1 line, import normalization | note 2 |
Verification of the delta
1. pipelines.py import normalization (note 2) — correct, and a genuine correctness fix, not cosmetic.
from orchestrator.message_store import … → from message_store import …. Python binds message_store and orchestrator.message_store as distinct module objects with distinct module-level globals. Before this change, the _probe_parent_branch_exists call site at pipelines.py:17391 got a separate get_message_store() singleton and a separate copy of the slice-6 _memory_fallback_degraded flag from the rest of the orchestrator. After normalization it shares the one flat module object that health.py:41 (import message_store as _message_store_module) and the other 69 from message_store import … sites already use. Verified:
- Zero
from orchestrator.message_store import …occurrences remain; 69 flat imports — fully consistent. - Imported symbols exist:
Message(message_store.py:135),get_message_store(:656). - The health surface reads
_message_store_module.is_memory_fallback_degraded()(health.py:137) off the same module object the degraded flag is now set on everywhere.
2. message_store.py comment (note 3) — factually accurate.
The added comment claims _store_lock is the real serialization guard and the once-flags' worst case is a duplicate log, never a missed degradation. Confirmed against the code: production reaches _create_message_store only via get_message_store()'s double-checked lock (:663-666), and _memory_fallback_degraded = True (:730) is set unconditionally before the if not _memory_fallback_logged guard, so the degraded set is monotonic and a racing direct caller can at most duplicate the log line. Accurate.
Regression check — slice-6 criteria still hold
The delta does not alter the fail-loud signal, the warning/error/silent matrix, the MEMORY_FALLBACK_MARKER, the health degraded field, or the test matrix. The import fix only makes the singleton + degraded flag coherent across the orchestrator, so it strengthens — rather than threatens — the previously verified slice-6 acceptance criteria. Both slice-6 tasks (task-6-1 coder, task-6-2 tester) remain complete.
Note on criterion marking
The orchestrator was unreachable this session, so egg-contract show / verify-criterion could not be run (the on-disk contract snapshot is a stale refine-phase copy with empty top-level criteria). The criteria verified in my prior review at 725b51e are unaffected by this delta; this re-review confirms no regression. The marker's machine-readable verdict below reflects the human-facing outcome.
Verdict
Approve. The delta is two narrow, correct review-feedback changes — one a no-op comment, one a correctness-strengthening import normalization. No contract violations; no regression to slice-6 criteria.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review feedback addressed (re-review delta 581ec33 → 82565b6)Both re-reviews approved. The one actionable note from the latest egg-reviewer[bot] re-review (581ec33)
james-in-a-box[bot] contract re-verification (581ec33)
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: slice-6 cascade-alert import (delta 581ec33 → 82565b6)
I deepened the shallow clone and reviewed the PR-authored delta against my prior review commit. The delta is exactly one change — the dual-form message_store import on the cascade-alert path that I flagged as non-blocking Note 1 in my last pass. No blocking issues. CI is fully green (Unit Tests, Integration Tests, Python, Security Scan, Lint all SUCCESS).
Note 1 (import-form consistency) — fixed correctly
routes/pipelines.py:17392-17398 now wraps the cascade-alert message_store import in the file's established dual-form:
try:
from message_store import Message, get_message_store
except ImportError:
from ..message_store import ( # type: ignore[no-redef]
Message,
get_message_store,
)I verified this is correct and not a regression:
- Matches the file convention exactly. Identical in structure to the other multi-symbol site at :15392-15394 (
Message, MessageType) and the single-symbol sites at :1985-1987 and :4386-4389, including the# type: ignore[no-redef]annotation. - Production singleton coherence is preserved. The flat form is tried first, so in the production runtime (where
pipelines.py's own top-level imports are flat andmessage_storeresolves onsys.path) the cascade alert still lands in the same module-global_message_storebuffer and shares the same_memory_fallback_degradedflag thathealth.py:41and the messages route read. The relative..message_storefallback only engages under package-import contexts (e.g. unit tests) where the flat form is unavailable — and there it is the only resolvable module object, so it's internally coherent. The split-brain seam from the original package-only form is not reintroduced. - Failure path unchanged. Still wrapped in the best-effort
except Exception: # noqa: BLE001 … pass, with the always-onlogger.warning("Slice cascade fired", …)above it as the durable fallback. The only behavioral change is that the alert now successfully emits under package-import contexts instead of silently falling through to the log line.
Grep confirms zero from orchestrator.message_store import statements remain in orchestrator/, so the singleton stays unified across all call sites.
Verdict
Approve. The delta is a single narrow, correct change that adopts the file's established import convention and addresses my prior non-blocking note. No new issues; no regression to the slice-6 fail-loud semantics, the degraded health field, or the test matrix.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract re-verification — incremental review
Re-reviewed the delta since my last verification at 581ec33. The PR head is now 82565b6, and the only PR-authored change since then is a single commit:
82565b6— Address review: adopt dual-formmessage_storeimport on cascade-alert path (orchestrator/routes/pipelines.py, +7/-1)
What changed
In _run_implement_phase_slices, the cascade-block OVERSEER_ALERT emission path previously used a bare top-level import:
from message_store import Message, get_message_storeIt now uses the dual-form pattern, falling back to a package-relative import:
try:
from message_store import Message, get_message_store
except ImportError:
from ..message_store import ( # type: ignore[no-redef]
Message,
get_message_store,
)Verification
- Pattern consistency — This matches the established dual-form
message_storeimport used throughout the same file (e.g. lines 1985–1987, 4386–4389, the_get_message_store()helper at 8726–8729, and 15392–15398 / 15478–15484). It removes the last odd-one-out bare import on this code path. - Import targets exist —
orchestrator/message_store.pyexports bothMessage(class, L135) andget_message_store(function, L656), so the..message_storefallback resolves correctly whether the module is loaded as a top-level module (message_storeonsys.path) or as a package submodule (orchestrator.routes.pipelines). - Defensive — The new inner
try/except ImportErroris nested inside the existing best-efforttry: … except Exception: passblock, so even a total import failure degrades to the always-onlogger.warninglog line rather than disrupting the slice loop. Sync/emission semantics are otherwise unchanged. - Compiles —
python -m py_compile orchestrator/routes/pipelines.pypasses.
Regression check
The change is scoped to a single import on a best-effort alert path and touches no other logic. No previously verified acceptance criterion is affected, and no contract task's described functionality is altered — this commit hardens existing code in response to a prior review comment rather than completing new work.
Note on criterion marking
The orchestrator is currently unreachable from this session (egg-orch health → Orchestrator: UNREACHABLE; gateway healthy), so egg-contract verify-criterion could not be invoked this cycle. This is not a blocker for the delta: the change introduces no new acceptance-criterion completion to mark — it is a hardening of existing best-effort emission code. The slice-6 criteria stand as previously assessed; nothing in this delta regresses them.
Verdict: No blocking issues. The delta is correct, minimal, and consistent with codebase conventions.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Review feedback addressed — current HEAD
|
|
egg feedback addressed. View run logs 14 previous review(s) hidden. |
… [doc-updater] (#3150) * docs: update coordination-state and deployment for #3077 slice-6 [doc-updater] Update documentation to reflect changes from #3144 (#3077 slice-6): - coordination-state.md: mark all six #3077 slice mechanisms as Shipped (slices 2-6 were still listed as Pending; all have now merged to main) - deployment.md: add `components.message_store` to the /api/v1/health response example and document the degraded case (MESSAGE_STORE_AUTO_FALLBACK_TO_MEMORY marker, what triggers it, and how to fix it) Triggered by: #3144 Authored-by: egg * docs: remove stale message_store health field after #3159 #3159 removed the in-memory message store backend, so the components.message_store health field and the auto->memory fallback no longer exist. Drop the JSON example line and the describing paragraph in deployment.md so it agrees with coordination-state.md and the current health.py response. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
auto→memory fallback fails loud (error-level structured log with a stable marker + health-visible degraded flag; explicit memory stays a warning); Redis restart semantics verified. Parallel root — file set disjoint from slices 1-5. Per HITL Q3: no refusal, no change to auto selection.
Base PR: #3139
What's in this PR
Commits (4):
This slice
Bounded durability: fail-loud memory-backend signal + Redis restart-semantics test
Files affected:
orchestrator/message_store.pyorchestrator/tests/test_message_store.pyorchestrator/tests/test_redis_message_store.pyTasks (2) + acceptance criteria
orchestrator/message_store.py_create_message_store()(backend selection ≈589-633): whenautoresolves to the in-memory fallback, emit an error-level structured log with a stable marker naming the mid-phase-restart loss risk (#3076 condition) and set a degraded flag visible on the orchestrator health surface. ExplicitEGG_MESSAGE_STORE_BACKEND=memory(dev/test intent) emits a warning, not an error, and no degraded flag is required. Per HITL Q3: warning/error + health flag only — do NOT refuse to run and do NOT changeautoselection semantics. Test-harness contexts must not be spammed.auto→memory fallback: exactly one error-level log with the stable marker + health-surface degraded field. - Explicit memory backend: warning level, no degraded flag. - Redis backend: neither. -autoselection behavior (redis-when-available, memory fallback) is unchanged; unit tests are not spammed.orchestrator/tests/test_message_store.pyfor the fail-loud matrix (auto→memory error + flag; explicit memory warning; redis silent) and extendorchestrator/tests/test_redis_message_store.pywith a restart-semantics test: mid-phase messages survive store re-instantiation against the same Redis (simulated orchestrator restart), while the DESIGNED phase-boundary wipe via_clear_concurrent_state()(orchestrator/routes/phases.py:113) still clears state. The two wipe semantics are asserted in the same module with explicit naming so accidental mid-phase loss (defect) cannot be conflated with the intentional wipe (required behavior)._clear_concurrent_state()still wipes at the phase boundary. - Fail-loud matrix asserted (error/warning/silent × flag). - Both wipe semantics named explicitly in test ids/docstrings.Stack
issue-3077egg/issue-3077/work