fix(cgka-engine): classify malformed group messages as terminal stale instead of aborting ingest#893
Open
jgmontoya wants to merge 5 commits into
Open
fix(cgka-engine): classify malformed group messages as terminal stale instead of aborting ingest#893jgmontoya wants to merge 5 commits into
jgmontoya wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesMalformed ingest handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jgmontoya
changed the title
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 28cb0c8 on Jul 17, 2026 4:59pm UTC. |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cgka-engine/tests/ingest.rs`:
- Around line 663-670: Update the assertion in the replay test around
alice.publish_failed and alice.ingest to require the specific terminal
IngestOutcome::PeelFailed variant, rather than accepting any
IngestOutcome::Stale result. Preserve the diagnostic message while ensuring
Retryable rows that lead to AlreadySeen cannot satisfy the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31e3d88b-8a5d-41f4-8ba3-76e590089591
📒 Files selected for processing (4)
crates/cgka-engine/src/message_processor/ingest.rscrates/cgka-engine/src/message_processor/mod.rscrates/cgka-engine/tests/ingest.rscrates/marmot-app/tests/malformed_445_drain.rs
jgmontoya
force-pushed
the
fix/malformed-445-terminal-stale
branch
from
July 17, 2026 02:56
ddc50f2 to
6bfeaa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cgka-engine/src/message_processor/ingest.rs`:
- Around line 1596-1607: Move
mark_raw_transport_message_failed_if_awaiting_retry and its durable
state-transition, audit-persistence, and deferred-row-accounting logic from
ingest.rs into the store/dedup layer in message_processor/store.rs. Expose a
store-layer helper with the required inputs, then update ingest callers to
invoke it while keeping ingest focused on peel, classify, and apply.
- Line 601: Update the WrongEpoch handling in the relevant message-processing
function so every terminal outcome retires the raw retry row identified by
raw_msg_id, including IncumbentWins, missing-snapshot fail-closed returns, and
the final AlreadyAtEpoch return. Apply
mark_raw_transport_message_failed_if_awaiting_retry consistently before those
returns, while preserving the existing content-derived msg.id updates and
too-distant-history behavior.
- Around line 383-403: Handle PeelerError::Malformed from
try_peel_group_message_from_available_snapshots at every fallback call site in
do_ingest using the same terminal behavior as the shown direct-path branch:
retire any awaiting retry row via
mark_raw_transport_message_failed_if_awaiting_retry, mark the message ID seen,
and return Stale with reason PeelFailed instead of propagating the error. Add a
regression test covering malformed content discovered only through a retained
snapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d35d2eaf-7398-40a5-8cc6-ad25d4009dba
📒 Files selected for processing (4)
crates/cgka-engine/src/message_processor/ingest.rscrates/cgka-engine/src/message_processor/mod.rscrates/cgka-engine/tests/ingest.rscrates/marmot-app/tests/malformed_445_drain.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/cgka-engine/src/message_processor/mod.rs
- crates/marmot-app/tests/malformed_445_drain.rs
- crates/cgka-engine/tests/ingest.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cgka-engine/src/message_processor/mod.rs`:
- Around line 883-897: Update the StaleReason::PeelFailed handling around
ingest_group_message so that when ingestion terminalizes the row as Failed, it
also releases the corresponding in-memory PeelDeferred flood-cap slot and
removes its attempt entry. Keep the cleanup atomic with the terminal state
transition to avoid torn bookkeeping, while preserving retry-state behavior for
rows that remain retryable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dbc1660-f77e-44e6-b24d-4a5dbc5146bf
📒 Files selected for processing (4)
crates/cgka-engine/src/message_processor/mod.rscrates/cgka-engine/src/message_processor/store.rscrates/cgka-engine/tests/fork_detection.rscrates/cgka-engine/tests/ingest.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cgka-engine/src/message_processor/store.rs
jgmontoya
force-pushed
the
fix/malformed-445-terminal-stale
branch
from
July 17, 2026 16:49
0fe8e07 to
3993185
Compare
… instead of aborting ingest
…r module split mark_raw_transport_message_failed_if_awaiting_retry is durable stored-message state, so it belongs with store.rs's persistence helpers, not the ingest peel/classify/apply path. Pure relocation, no behavior change.
…l stale instead of aborting the drain The two snapshot-fallback call sites propagated a PeelerError::Malformed with ? , aborting the whole transport drain on one garbage event. Give them the direct peel's terminal handling via a shared malformed_terminal_stale helper. Defense-in-depth for the public TransportPeeler contract, not a production-reachable Nostr bug; closes the one-seam guard gap (mdk#707).
…comes Closes a pre-existing gap in replay_buffered_messages where a raw transport row buffered Retryable during our own PendingPublish/Merging window was never retired when re-ingest returned a terminal Ok outcome (Processed, AlreadySeen, AlreadyAtEpoch/EpochInvalidated, OwnEcho, SelfEvicted): it lingered Retryable and was re-peeled on every later publish-cycle replay. The Ok(_) arm now retires the raw wrapper Processed (cap-slot release stays gated on PeelDeferred, so no mdk#339 accounting impact); UnknownGroup moves to the leave-in-retry arm since ingest deliberately re-buffers it. The retirement is guarded on the row's re-read current state so it fires only while the row is still awaiting retry: a terminal state ingest already committed to the same row during re-ingest (e.g. SelfEvicted persisting it Failed) is authoritative and is never clobbered to Processed.
…ering ingest-committed terminal state Seam parity with replay_buffered_messages (5ae9a44). The deferred-peel sweep snapshots its PeelDeferred rows once, then re-ingests each via ingest_group_message. Both terminal-outcome arms (Ok(Buffered|Processed) and the Ok(Stale{..}) catch-all) retired the raw wrapper Processed UNCONDITIONALLY. But ingest can commit a terminal state to the same row during the call: a deferred future-epoch commit that re-ingests against a now-inactive group hits the !is_active gate, which persists the row Failed (SelfEvicted, ingest.rs). Relabeling that evicted-on row Processed swept it back into canonicalization (openmls_projection / distributed_convergence select on Processed); a fork-recovery rollback that deletes the row mid-sweep would instead crash the write with Storage(NotFound). Both writes now re-read the row's current state through the existing raw_transport_row_awaiting_retry helper and retire only a state ingest left awaiting retry, so ingest's terminal verdict is authoritative. The flood-cap release (note_peel_deferred_row_retired) stays outside the guard, mirroring 5ae9a44: the row leaves the retry lifecycle regardless of who terminalized it. The budget-terminal arm (runs before this row's ingest) and the ForkedEpoch arm (forked_epoch_fail_closed already wrote the row EpochInvalidated, then returns) are left unchanged.
jgmontoya
force-pushed
the
fix/malformed-445-terminal-stale
branch
from
July 17, 2026 16:58
3993185 to
28cb0c8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit