Add structured local failure history for conductor jobs#525
Open
morluto wants to merge 6 commits into
Open
Conversation
Implement versioned, bounded failure records and a read-only query surface for recent build/test failures. The store is thread-safe, writes atomically via temp files + os.replace, and preserves schema-compatible defaults for missing fields. Retention only considers *.failure.json and *.summary.json so in-flight temp files are not deleted. Aggregate JSON output includes an explicit privacy note stating that raw logs, source content, prompts, transcripts, credentials, and environment dumps are excluded.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Comment on lines
+2586
to
+2589
| with self.condition: | ||
| current.failure_record_pending = False | ||
| self._append_system_line_locked(job, f"failure record write failed: {exc}\n") | ||
| return |
There was a problem hiding this comment.
Bug: The method _append_system_line_locked is called without holding the required self.condition lock in an exception handler, creating a race condition when modifying job.tail.
Severity: MEDIUM
Suggested Fix
The call to _append_system_line_locked within the except block of _refresh_output_summary should be moved inside the with self.condition: block that is already present for setting current.failure_record_pending = False. This will ensure the lock is held during the modification of the shared job.tail deque.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: Scripts/conductor.py#L2586-L2589
Potential issue: In the `_refresh_output_summary` method, if writing a failure record to
the `failure_store` throws an exception, the code enters an `except` block. Inside this
block, it calls `_append_system_line_locked` to log the error. However, this call is
made without acquiring the `self.condition` lock. The `_append_system_line_locked`
method modifies the shared `job.tail` deque, and doing so without synchronization in a
multi-threaded context can lead to race conditions and data corruption. The method's
name explicitly suggests a lock is required.
Did we get this right? 👍 / 👎 to inform future reviews.
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
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.
Why
Conductor preserves raw logs and terminal summaries, but contributors cannot group recent failures by cause without repeatedly parsing those logs. Storing raw output in a new aggregate would also duplicate sensitive data.
This PR adds a bounded local record containing safe job metadata and references to existing local artifacts.
Closes #423.
What changes
FailureRecordschema../conductor diagnostics recent-failureswith operation, class, age, and limit filters.Privacy boundary
Aggregate records exclude raw logs, source contents, prompts, transcripts, credentials, environment dumps, and arbitrary operation arguments. Only explicitly safe arguments and allow-listed toolchain metadata remain.
Persistence design
This is local developer diagnostics, not telemetry. No remote reporting path is added.
Review order
Verification