Repository navigation
fix(ingest): break infinite-Nak loop on permanent delete errors - #122
Conversation
Phase 1 of issue #91 — every chConn.Exec failure on an `action: "delete"` message used to trigger m.Nak(), which JetStream interprets as "redeliver immediately." A deterministic error (syntax, unknown table, malformed identifier) would loop forever — clogging the buffer consumer, burning CPU, and spamming logs with the same message. Now the worker treats every delete-Exec error as permanent: it publishes the original NATS envelope to dlq.<table> (reusing the bentoDLQDropped counter when the DLQ publish itself fails) and DoubleAcks so the message leaves the main queue. Phase 2 (transient vs permanent classification — timeouts and network errors should still Nak) remains as follow-up on the issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where deterministic errors during delete operations in the Bento ingest pipeline caused infinite retry loops, leading to log spam and resource exhaustion. By reclassifying these errors as permanent and routing the affected messages to a dead-letter queue, the system now gracefully handles malformed delete requests while ensuring the main queue continues processing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes |
github-actions
Bot
added
documentation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
0 [MUST], 0 [SHOULD], 1 [MAY] — see inline thread for detail. The fix is correct. The infinite-Nak root cause is real, the Phase 1 permanent-error policy is a sound stopgap, the new Ship it — the only thing to chase down is opening a Phase 2 tracking issue before #91 auto-closes on merge. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #91 by preventing infinite retry loops for permanent delete errors in the ingest worker. Specifically, in internal/ingest/bento.go, failed delete operations are now treated as permanent; the original message is routed to a table-specific DLQ subject and then DoubleAck'd to remove it from the main queue. The changes include updates to the jsInput struct to include a JetStream client, logic to handle DLQ publishing, and comprehensive test coverage for these failure scenarios. Documentation in docs/architecture.md and the CHANGELOG.md have been updated accordingly. I have no further feedback to provide.
Addresses Claude review feedback on PR #122: - CHANGELOG.md no longer says "Closes #91" so the issue stays open after merge to track Phase 2 (transient-vs-permanent error classification). - AGENTS.md §Architecture's `ingest/` quick-reference now mentions the permanent-error policy on delete failures (DLQ + DoubleAck, not Nak). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for the review — all 1 MAY addressed in 26ba089:
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
|
0 [MUST], 0 [SHOULD], 0 [MAY] — follow-up commit 26ba089 correctly addresses the one finding from the prior round. AGENTS.md No new findings in the follow-up commit. Ship it — pending CI green and admin approval. |
|
Thanks Gemini — noting the clean review, no follow-up required on my end. The Phase 1 trade-off (all delete errors → permanent) is intentional per issue #91; Phase 2 (transient-vs-permanent classification so timeouts and network blips still Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
|
Thanks for the update, Eric. The changes in 26ba089 look solid and correctly address the documentation and tracking requirements discussed. The implementation of the permanent-error policy for delete operations, along with the robust handling of DLQ publish failures, effectively mitigates the infinite-retry loop issue. The added test coverage in |
|
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:
📝 WalkthroughWalkthroughThe ingest worker treats ClickHouse DELETE Exec failures as permanent: it publishes the original envelope to ChangesPermanent Delete Failure Handling
Sequence DiagramsequenceDiagram
participant IngestWorker
participant ClickHouse
participant JetStream
IngestWorker->>ClickHouse: Execute DELETE
ClickHouse-->>IngestWorker: Exec error
IngestWorker->>JetStream: Publish original envelope to dlq.<table> (with Wave-DLQ-Type header + trace)
IngestWorker->>IngestWorker: DoubleAck original message
IngestWorker->>IngestWorker: Continue to next message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CHANGELOG.md`:
- Around line 9-10: The "### Fixed" heading in CHANGELOG.md violates
markdownlint MD022; add one blank line immediately above the "### Fixed" heading
and one blank line immediately below it so the heading is separated from
surrounding text, then commit the change (no code changes required beyond
editing the CHANGELOG.md heading spacing).
In `@docs/src/content/docs/architecture.md`:
- Around line 159-161: The documentation is inconsistent: the flow currently
says "Ack messages" but the implementation uses DoubleAck for successful
inserts; update the wording by replacing the "Ack messages" phrase with
"DoubleAck" so the doc matches the implementation (search for the exact string
"Ack messages" and change it to "DoubleAck" in the same flow, keeping the
surrounding lines about routing to dlq.{table} and the Phase 1 note intact).
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 17b4e3a1-9ebb-4495-b261-8e2107e8c3a9
📒 Files selected for processing (5)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/architecture.mdinternal/ingest/bento.gointernal/ingest/bento_test.go
github-project-automation
Bot
moved this from In review
to Ready
in WaveHouse Task Board
|
0 [MUST], 1 [SHOULD], 0 [MAY] — see inline thread for detail. The fix is correct and the design is sound. The infinite-Nak root cause is real, the Phase 1 permanent-error policy is an appropriate stopgap, and the two new test cases cover exactly the invariants that matter: no Nak on exec error, DoubleAck unconditionally regardless of DLQ publish outcome. One [SHOULD]: Ship it — the [SHOULD] is documentation only and can be addressed with a one-line comment or a short addition to architecture.md before merge. |
…k wording Addresses bot-review feedback on PR #122: - bento.go: comment above the delete-DLQ Publish noting the message-shape difference vs dlqOutput.WriteBatch — insert failures publish raw.Payload (inner data only), delete failures publish m.Data() (full envelope) so a consumer can re-issue the delete. Phase 2 (issue #91) may normalize. - docs/src/content/docs/architecture.md: explicit DLQ-shape contract on the ingest/ blurb so future consumers find it; and fix the data-flow line that read "Ack messages" — the code uses DoubleAck. - CHANGELOG.md: blank line above the new "### Fixed" heading (MD022). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@CHANGELOG.md`:
- Line 11: Update the changelog entry to reference the current docs path by
replacing the stale `docs/architecture.md` string with
`docs/src/content/docs/architecture.md` in the CHANGELOG.md entry (the line
describing Ingest worker changes that currently includes
`docs/architecture.md`); ensure the updated path appears exactly in the same
list of affected files so the changelog points to the correct documentation
location.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e21d8a07-8870-4868-9039-6e3cf14e2ddc
📒 Files selected for processing (3)
CHANGELOG.mddocs/src/content/docs/architecture.mdinternal/ingest/bento.go
|
0 [MUST], 0 [SHOULD], 0 [MAY] — no inline threads. Head commit 34f742f correctly addresses all findings from prior rounds:
The fix itself remains sound: the infinite-Nak root cause was real, the Phase 1 permanent-error policy is an appropriate stopgap, Ship it — pending CI green and admin approval. |
…re.md Stale path artifact from the merge — main moved docs/ to Astro layout under docs/src/content/docs/ but my Phase-1 CHANGELOG entry still listed the pre-merge path. Caught by CodeRabbit on the round-2 review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
0 [MUST], 0 [SHOULD], 0 [MAY] — no inline threads. Both findings from the prior round (commit
The core fix remains sound and has been extensively reviewed: Ship it — pending CI green and admin approval. |
Two refinements from Claude's review of 55da248 on PR #122: pollCount in ingest_test.go used fmt.Sprintf with single-quote id interpolation and a raw (unquoted) table identifier. Current callers pass controlled constants so there's no live injection risk, but the function signature accepts arbitrary strings and would be copied verbatim by future test authors. Switch to the same pattern bento.go:172 uses in production: backtick-quoted table identifier + driver-bound ? parameter for the id. Comment notes why the table can't be a bound parameter (CH identifiers aren't bindable; backticks are the same defense the production path uses). TestDelete_FailureRoutesToDLQWithHeader's no-redelivery assertion used NumAckPending == 0, which is a global claim about the entire buffer consumer rather than a claim about our specific message. If some unrelated test left a Nak'd message whose AckWait hadn't elapsed, this would time out with a misleading "must DoubleAck the failed delete" error. Switch to capturing pubAck.Sequence from js.Publish and asserting AckFloor.Stream >= pubAck.Sequence — that targets THIS message's stream seq, immune to global consumer state. The pre/post Delivered snapshot is no longer needed; removed. Per Claude on PR #122 / commit 55da248. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
There was a problem hiding this comment.
Good. I was looking for the DoubleAck call because that is also what I had done to solve part of this issue the first time I ran into it. Now we have SQL DELETE support for te ingest worker, using an in flight drain approach to make sure pending insert batches finish before a delete executes. To prevent infinite retry loops on malformed queries, failed deletes now follow a permanent-fail policy, so bypassing redelivery and routing directly to the NATS DLQ.
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
…ator After merging main's boot-resilience and health work (#125, #122), e2e coverage dipped to 49.9% (gate is 50%) because the Readiness handler and the cmd/wavehouse/health.go probe binary were uncovered by the SDK harness. Both are production code paths the operator-facing contract (k8s readiness, Docker HEALTHCHECK) depends on — exercising them in the e2e harness is principled, not a coverage hack. Brings e2e from 49.9% → 50.9%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Summary Umbrella PR setting up shared Claude Code + AI agent infrastructure for the WaveHouse team. Two work streams: 1. **AI rules drift cleanup** — corrected stale references that AI tools (Claude Code, Gemini Code Assist, Copilot, CodeRabbit) were following blindly. 2. **Claude Code native tooling** — committed `.claude/` configuration and `.githooks/` so every teammate gets identical dev affordances out of the box, with agent-specific gating layered on top. The team just got Max 20x subscriptions across the board; this lands the team-wide config so everyone is on the same agentic dev experience by default. ## Scope ### 1. AI rules drift cleanup - **17 doc-path references corrected** across AGENTS.md + CONTRIBUTING.md to `docs/src/content/docs/*.md` (the actual Astro Starlight location, not the old flat layout). - **`.github/copilot-instructions.md` shrunk to a pointer** — was drifting on Go 1.25 (vs current 1.26.3) and 60% coverage (vs current 80% total / 70% unit per `.testcoverage.yml`). - **`.gemini/styleguide.md`** — stale `#67` / 60% claim fixed (issue closed, 70% restored); duplicated doc-sync bullet collapsed to defer to AGENTS.md (already authoritative). - **`.github/labeler.yml`** — dropped non-existent `cmd/wavehouse-{api,worker}/**` entries; fixed `tests/{compose.yaml,sdk/**}` → `tests/e2e/...`; added `cmd/wavehouse/**` to `area/infra`. - **`.github/prompts/pr-review.md`** — doc-sync list collapsed; vestigial "tenant" wording dropped (no tenant model in WaveHouse); hard-wrap reflowed (180 → 81 lines). - **AGENTS.md** — `cmd/*/main.go` (plural) → `cmd/wavehouse/main.go` (one binary); `tests/fixtures/` → `tests/e2e/fixtures/`; removed stale "update `triage.yml` area enumeration" step (workflow now discovers `area/*` labels dynamically); fixed internal-package count. - **CONTRIBUTING.md** — vestigial "tenant isolation" wording removed. - **TODO.md deleted** — audit summary below. ### 2. Claude Code native tooling - **`.claude/`** — shared configuration: `settings.json` (deny rules + worktree config + three hooks wired), `agents/pre-push-reviewer.md`, `hooks/agent-bash-gate.sh` (PreToolUse Bash gate), `hooks/review-marker.sh` (PostToolUse Agent marker writer), `hooks/gofumpt-on-save.sh` (auto-format), `skills/pr-review-locally/`, `skills/pr-sync-with-main/`, `commands/cover.md`. - **`.githooks/`** — universal team hooks installed by `make tools`: `pre-commit` runs `make verify`; `pre-push` requires `tmp/ci-passed-<HEAD-sha>` marker (written by `make ci`). - **`.config/wt.toml`** — worktrunk project hooks so parallel-agent worktrees install `.githooks/` correctly. - **AGENTS.md §"Agent PR Discipline"** — new section codifying the agent-only ruleset: - Drafts-only PR creation; human-only ready/approve/request-changes/reviewer-add transitions. - Bot reviewer re-triggers go through PR comments (`@coderabbitai review`, `@gemini-code-assist`, `@claude` / `/review`). - **Pre-push self-review mandatory** on PR branches: agent invokes `pre-push-reviewer` subagent in fresh context. `ship_it` requires zero findings at any severity — any `[MUST]` / `[SHOULD]` / `[MAY]` forces iterate; the orchestrator loops review → fix → review until clean. - **Honest-agent marker policy**: `--no-verify` regex-blocked + the obvious marker-write idioms denied at the permission layer (`Bash(touch tmp/ci-passed:*)`, `Write`/`Edit` on the canonical paths); everything else is a documented rule, not regex-enforced. Bash can write a file by a dozen paths and regex enforcement is a porous game of whack-a-mole. - **`docs/src/content/docs/claude-code.md`** — contributor-facing page documenting the four-layer model (universal git hooks → agent gate → ergonomic hooks → skills/agents/commands), quick setup, and discipline rules. - **CHANGELOG.md** — `[Unreleased]` entry covering all of the above. ## Out-of-tree GitHub changes that pair with the AI-rules cleanup Done via `gh` CLI as part of the same audit: - **Closed #46** (Graceful Shutdown) — verified shipped in `cmd/wavehouse/main.go:378-393` (SIGINT/SIGTERM → bounded shutCtx → ingestStream.Stop → srv.Shutdown → promSrv.Shutdown). - **Scope notes added to #44, #50, #94** with current-status / boundary info (ldflags shipped vs `/version` remaining; DLQ shipped vs retry remaining + scope boundary with #91; per-component logger source field as a #94 complement). - **Opened 4 new issues from orphan TODOs**: #143 (pprof), #144 (K8s `/healthz` + per-dep health), #145 (RequireRoles fail-closed), #146 (split `internal/api/` into focused subpackages). ## TODO.md audit (one-time, for record) | Bucket | Count | Disposition | |--------|-------|-------------| | Already shipped per closed issues (#11, #14, #16, #28, #40, #41, #42, #45) + current code | ~12 | Deleted from TODO | | Tracked as open issues (#32, #33, #34, #37, #39, #44, #48, #49, #50, #51, #94) | ~12 | Kept as issues, scope notes added where useful | | In-flight via open PRs (#83, #92, #119, #122, #125, #136, #137) | 4 | Untouched | | #46 Graceful Shutdown | 1 | Verified shipped, closed with comment | | Orphan items | 4 | Split into #143-146 | | Aspirational ("more tests", "update README") | 2 | Deleted — covered by AGENTS.md doc-sync rules | Projects #7 board + triage automation is now the single canonical backlog. ## Test plan - [x] `make ci` passes locally for each push (gated by `.githooks/pre-push`) - [x] CI green on the latest HEAD (8fbd7db) - [x] PR-title-lint accepts the title (`chore: claude code native improvements`) - [x] All `docs/src/content/docs/*.md` paths in AGENTS.md resolve to real files - [x] Labeler workflow auto-labels correctly per the updated paths - [x] `pre-push-reviewer` subagent loop reached `VERDICT: ship_it` with zero findings under the strict rubric before the final push (validated end-to-end across five iterations on this branch — each surfacing a real doc-sync / off-by-one / quote-strip issue and forcing a fix before the marker auto-wrote) - [x] `agent-bash-gate.sh` quote-strip generalization sanity-tested live: `echo "git push to deploy"` passes through; `git push --no-verify` and `git commit --no-verify` still block (`bash -n` clean, JSON wiring valid) - [ ] Human review ## Related issues - Closed during this work: #46 - Scope notes added: #44, #50, #94 - New follow-up issues created: #143, #144, #145, #146 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Claude Code integration: local pre-push reviewer with strict ship/iterate/block verdicts, push gating via CI/review markers, automatic review-marker creation, and a coverage-reporting command. * **Documentation** * Comprehensive Claude Code & agent docs, new skill guides for PR review/sync, updated README/CONTRIBUTING/CHANGELOG/styleguide, and site sidebar/page additions. * **Chores** * Added git and agent hooks, CI marker creation, worktrunk config, labeler tweaks, and simplified Copilot instructions. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/Wave-RF/WaveHouse/pull/147?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
](https://app.coderabbit.ai/change-stack/Wave-RF/WaveHouse/pull/147?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
2 participants
Summary
Phase 1 of issue #91 — addresses Phase 1 of #91. Does not close the issue: #91 stays open after this lands so Phase 2 (transient-vs-permanent error classification) has a visible tracker.
jsInput.Read'saction: "delete"block calledm.Nak()on everychConn.Execfailure, which JetStream interprets as "redeliver immediately." A delete whose error was deterministic (syntax error, unknown table, malformed identifier) looped forever — clogging the buffer consumer, burning CPU, and spamming logs with the same message.This change treats every delete-Exec error as permanent:
ERRORwith table, id, and underlying error.m.Data()) todlq.<table>so operators can inspect what failed. Reuses the existingbentoDLQDroppedcounter if the DLQ publish itself fails.m.DoubleAck(ctx)so the message is removed from the main queue.continues to the next message (no morereturn … fmt.Errorf("execute delete: %w", err)propagating into Bento's reconnect path).Phase 2 — distinguishing transient errors (timeouts, network) which should still
Nak()for retry from permanent ones (syntax, unknown table) — is left for a follow-up PR. Issue #91 remains the tracker.Files changed
internal/ingest/bento.go— new behavior in the delete-error branch +js jetstream.JetStreamthreaded intojsInput.internal/ingest/bento_test.go—TestJsInput_Read_DeleteExecErrorrewritten to assert the new behavior; newTestJsInput_Read_DeleteExecErrorDLQPublishFailscovers the DLQ-publish-also-fails path (must still DoubleAck).docs/architecture.md— package summary + ingest data-flow updated.AGENTS.md—ingest/quick-reference mentions the permanent-error policy.CHANGELOG.md— Fixed entry under[Unreleased].Test plan
make cipasses locally (unit 73.4%, integration 17.2%, e2e 51.6%, sdk 53.2%, merged 81.1%)internal/ingestcoverage 85.6% — new tests cover the success path (msg routed to DLQ + DoubleAck) and the failure path (DLQ publish errors → still DoubleAck'd to break the loop)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests