Failure Detector Expansion 1/5: Read failure type from updated valkey-io/valkey artifact and group nameless failures - #79
Conversation
BChan-0
changed the title
BChan-0
changed the title
f6105bc to
fc21793
Compare
BChan-0
changed the title
BChan-0
changed the title
fc21793 to
8e011ca
Compare
📝 WalkthroughWalkthroughThe failure detector now classifies and normalizes multiple failure types, merges same-run failures, renders stable issue content, and tracks recurrence traces. Issue deduplication uses cached bodies and claim guards. Artifact listing now supports filtered pagination with validation and limits. ChangesFailure detection and issue publishing
Issue deduplication safeguards
Workflow artifact retrieval
Sequence Diagram(s)sequenceDiagram
participant FailureReports
participant parse_and_deduplicate
participant process_failures
participant issue_renderer
participant IssueDedupPublisher
FailureReports->>parse_and_deduplicate: classify and deduplicate failure entries
parse_and_deduplicate->>process_failures: return UniqueFailure records
process_failures->>issue_renderer: build fingerprints, titles, bodies, and traces
issue_renderer->>IssueDedupPublisher: submit IssueContent by marker namespace
IssueDedupPublisher->>FailureReports: create or update the reported issue
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (1)
scripts/test_failure_detector/issue_renderer.py (1)
574-578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
_tool_labels_forsince it has no callers.
_tool_labels_foris no longer used by the rendering paths, so remove the helper to avoid leaving code that describes a body shape the renderer no longer writes.🤖 Prompt for 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. In `@scripts/test_failure_detector/issue_renderer.py` around lines 574 - 578, Remove the unused _tool_labels_for helper and its associated implementation, leaving the existing rendering paths and related symbols unchanged.
🤖 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 `@scripts/common/issue_dedup.py`:
- Around line 189-202: Update the creation-comment handling around
issue.create_comment so it performs only one best-effort attempt instead of
passing the non-idempotent write through retry_github_call. Preserve the
existing exception handling and warning behavior for failures, including the
issue number and traceback.
- Line 247: Update the issue deduplication flow around the existing edit and
comment creation calls so _record_body(repo_name, existing.number, new_body)
runs immediately after existing.edit succeeds, before existing.create_comment;
remove the later recording call to avoid duplicate or stale-state handling.
---
Nitpick comments:
In `@scripts/test_failure_detector/issue_renderer.py`:
- Around line 574-578: Remove the unused _tool_labels_for helper and its
associated implementation, leaving the existing rendering paths and related
symbols unchanged.
🪄 Autofix
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 Plus
Run ID: 2420fd94-05d7-46a1-b9a9-6adb2dcb7f05
📒 Files selected for processing (9)
scripts/common/issue_dedup.pyscripts/common/workflow_artifacts.pyscripts/test_failure_detector/issue_renderer.pyscripts/test_failure_detector/manage_issues.pyscripts/test_failure_detector/parse_failures.pytests/test_issue_dedup.pytests/test_testfailuredetector_failure_parser.pytests/test_testfailuredetector_issue_manager.pytests/test_workflow_artifacts.py
Signed-off-by: Bonnie Chan <bonniecv@amazon.com>
8e011ca to
c34d1e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/test_failure_detector/issue_renderer.py (3)
684-684: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_ROW_HEADING_REdoes not match theSummaryandFailing test(s)headings the body writes.
_build_bodyemits**Summary**and**Failing test(s)**at lines 616 and 620, without the colon inside the asterisks. The pattern requires**Summary:**and**Failing test(s):**, so those two alternatives never defuse a trace line that copies a real heading. Only theEnvironmentsalternative matches a heading the body actually writes, which is also the only heading a reader is anchored to today.Either align the pattern with the emitted headings, or drop the two unused alternatives and state that only the Environments row needs defusing.
🤖 Prompt for 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. In `@scripts/test_failure_detector/issue_renderer.py` at line 684, Update _ROW_HEADING_RE to match the exact heading format emitted by _build_body for Summary and Failing test(s), placing the colon outside the closing markdown emphasis, or remove those alternatives if only Environments requires defusing. Preserve matching for the actual Environments heading.
198-224: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueA trace can be recorded as published while the comment drops its text.
Each fresh trace is truncated at the full
_MAX_TRACE_CHARSbudget and its digest is appended topublished. The traces are then joined at line 224 and_build_commenttruncates the joined string again at the same budget (line 838). If two fresh traces are large, the join loses the tail, but both digests are recorded, soreportedsuppresses the dropped trace on every later run.Consider truncating each fresh trace to a share of the budget before joining, the same way
_build_absorbed_trace_commentdivides it, and digesting the form actually published.🤖 Prompt for 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. In `@scripts/test_failure_detector/issue_renderer.py` around lines 198 - 224, Adjust the fresh-trace handling in the renderer loop so each candidate is truncated to its share of the final comment budget before backtick bounding and digesting, matching the per-trace allocation used by _build_absorbed_trace_comment. Ensure fresh contains the same bounded forms that the joined output publishes, so _published_digests only records trace text retained in the final comment.
952-955: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead
_extract_error_from_body.
_extract_error_from_bodyhas no callers. Keep_extract_errors_from_bodyas the supported extractor, remove_extract_error_from_body, and update the stale references at lines 720 and 764.🤖 Prompt for 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. In `@scripts/test_failure_detector/issue_renderer.py` around lines 952 - 955, Remove the unused _extract_error_from_body helper, retain _extract_errors_from_body as the supported extractor, and update the stale references near the issue-rendering logic to call the plural helper directly.
🤖 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 `@scripts/test_failure_detector/issue_renderer.py`:
- Line 252: Update _COUNT_RE so the byte(s) and object(s) alternatives match
when followed by whitespace or end of input, removing the invalid word-boundary
requirement after the closing parenthesis. Preserve the existing matching
behavior for bytes, blocks, and their plural forms.
---
Nitpick comments:
In `@scripts/test_failure_detector/issue_renderer.py`:
- Line 684: Update _ROW_HEADING_RE to match the exact heading format emitted by
_build_body for Summary and Failing test(s), placing the colon outside the
closing markdown emphasis, or remove those alternatives if only Environments
requires defusing. Preserve matching for the actual Environments heading.
- Around line 198-224: Adjust the fresh-trace handling in the renderer loop so
each candidate is truncated to its share of the final comment budget before
backtick bounding and digesting, matching the per-trace allocation used by
_build_absorbed_trace_comment. Ensure fresh contains the same bounded forms that
the joined output publishes, so _published_digests only records trace text
retained in the final comment.
- Around line 952-955: Remove the unused _extract_error_from_body helper, retain
_extract_errors_from_body as the supported extractor, and update the stale
references near the issue-rendering logic to call the plural helper directly.
🪄 Autofix
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 Plus
Run ID: 0f4dc6db-d70b-49ee-bba1-66153f4eb101
📒 Files selected for processing (3)
scripts/common/issue_dedup.pyscripts/test_failure_detector/issue_renderer.pytests/test_issue_dedup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_issue_dedup.py
| # Allocation sizes drift run to run for the same leak ("49 bytes" vs | ||
| # "52 bytes"), so titles show them as N: "N bytes in N blocks are | ||
| # definitely lost". | ||
| _COUNT_RE = re.compile(r"\b\d[\d,]*(\s+(?:bytes?|blocks?|byte\(s\)|object\(s\)))\b") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_COUNT_RE never matches the byte(s) and object(s) forms.
The trailing \b follows a literal ). A word boundary after ) requires a word character next, but these phrases are followed by a space or end of line. So "41 byte(s)" and "3 object(s)" keep their counts, and a title built from such a line changes whenever the magnitude changes.
The bytes? and blocks? alternatives are unaffected because they end in a word character.
🐛 Proposed fix
-_COUNT_RE = re.compile(r"\b\d[\d,]*(\s+(?:bytes?|blocks?|byte\(s\)|object\(s\)))\b")
+_COUNT_RE = re.compile(r"\b\d[\d,]*(\s+(?:bytes?\b|blocks?\b|byte\(s\)|object\(s\)))")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _COUNT_RE = re.compile(r"\b\d[\d,]*(\s+(?:bytes?|blocks?|byte\(s\)|object\(s\)))\b") | |
| _COUNT_RE = re.compile(r"\b\d[\d,]*(\s+(?:bytes?\b|blocks?\b|byte\(s\)|object\(s\)))") |
🤖 Prompt for 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.
In `@scripts/test_failure_detector/issue_renderer.py` at line 252, Update
_COUNT_RE so the byte(s) and object(s) alternatives match when followed by
whitespace or end of input, removing the invalid word-boundary requirement after
the closing parenthesis. Preserve the existing matching behavior for bytes,
blocks, and their plural forms.
1 participant
This is the first of five PRs expanding the Failure Detector past assertion failures. It carries the schema and dedup plumbing the other four build on; each of those adds one failure category and can merge in any order once this lands. The producer side is valkey PR 4292, which adds the
typefield to the artifact.Issue
The detector only ever files issues for assertion failures. Valkey's Daily also produces timeouts, sanitizer and valgrind reports, server startup failures, memory-leak reports, uncaught test-client exceptions, and gtest unit-test failures, and none of them reach an issue.
'parse_and_deduplicate` drops any entry that does not name both a test and a file
A valgrind or sanitizer report has no test name because the tool doesn't run within tests. The same is true of a startup failure and a leak report. Those entries are read, discarded, and the run reports no failure for a job that was red.
PR Summary
FailureType declares all eight types up front (assertion, sanitizer, valgrind, timeout, startup, exception, memory-leak, unittest), so the later PRs add handling rather than enum values and producer/consumer drift stays visible. The type field defaults to assertion when absent, which keeps the current artifact format working, and an unrecognized value falls back to exception with a warning in the run log rather than being dropped.
A nameless failure now groups on its normalized error identity instead of being skipped. Normalization removes what changes between runs of one bug: PIDs, hex addresses, temp paths, timestamps, byte and block counts, loss records, and the runner's workspace layout, which differs per platform and was splitting one root cause into an issue per OS. A nameless timeout is the exception and groups on its test file, since every timeout carries the same generic error text and would otherwise collapse into one issue for the whole run.
Where an error has a stack, the identity anchors on it, so two leaks with identical report lines but different allocation sites stay distinct. Only the frame that names the bug keeps its line number: the frames below it are callers whose lines move whenever unrelated code is edited, and carrying the whole chain refiled one leak on most commits. Allocation wrappers in zmalloc.c and sds.c are skipped by name, since whether they appear at all depends on the compiler's inlining rather than on the bug. They are matched by name rather than by file because those files also hold ordinary code that can itself be the faulting frame, and skipping the whole file made a bad write in sdscatlen and one in sdsrange reduce to the same anchor.
Each of the eight types gets its own marker namespace, so two types cannot collide on one issue. All of them share the [TEST-FAILURE] title prefix and name the type in the body instead; a per-type prefix would rewrite an issue's title whenever the same bug was re-attributed, which happens when valgrind and a sanitizer report it on different days. A trace is embedded in the body, so text in it that looks like the body's own structure is now defused.
ArtifactClient.list_run_artifacts paginates and takes a server-side name filter.
Testing
Added:
1455 total tests passed.