fix: make silent job and peer failures visible and actionable - #9497
fix: make silent job and peer failures visible and actionable#9497pjwerneck wants to merge 15 commits into
Conversation
- jobs[N] now matches the [N] shown in the table; jobs["name"] raises on an ambiguous name instead of returning the wrong job - process_approved_jobs reports every approved job it skipped, and skips on (job_name, ds_email) so same-named jobs from other submitters still run - syft_job, syft_rds, syft_enclaves and syft_bg configure their own loggers - "No public encryption bundle" names the cause and remedy per peer state - approve_job checks for the approval file first; JobInfo file listings catch OSError; approve/reject errors name both parties
github-actions
Bot
added
bugfix
pkg:syft-enclave
pkg:syft-bg
pkg:syft-job
pkg:syft-rds
pkg:syft
labels
chore: land syft-pr-review skill update on this branch for testing
chore: land prefetch.sh .claude exclusion on this branch
Both were code standards that already said the right thing and were applied too loosely. The naming standard covered test names only; it now covers any name, and pins the output to the name, what it breaks, and the rename — nothing else. The duplication standard said 'functions or methods' and was still fired on a couple of repeated lines, so it now says so outright: it never fires on lines.
chore: land naming/duplication standard update on this branch
… findings Two over-corrections from the previous commit. The duplication floor said 'never on lines', which is stronger than intended: a repeated line or two is not worth flagging, but a big enough copied block still is, whether or not it is a whole function. The naming finding was pinned to an 'old - violation - new' dash form. A plain sentence is what was asked for: 'x() violates the no-filler rule, rename to y()'.
chore: land naming/duplication corrections on this branch
| def job_sort_key(job): | ||
| # Parse submitted_at timestamp for sorting (most recent first) | ||
| # Root owner first, then peers, newest-first within each owner. | ||
| # This list is the authority for both jobs[N] and the table's [N]. |
There was a problem hiding this comment.
JobClient.jobs is now the single authority for order — own datasite first, then owner email, then newest — and both the printed [N] and jobs[N] read that one list, so they agree.
I think in general we need 1) a deterministic way to get job X from user Y, ideally client.jobs["a@b.org"]["<jobname>"], and 2) to use that in the hints.
There was a problem hiding this comment.
because fixing the hints kind of solve a problem that shouldnt exist in the first place: no deterministic way to get a job handle, we may sometimes use the shorthand but we shouldnt all the time. If we can assert that there is no @ in a job name, we would know whether a passed arg is an email or a job name, filter on emails first and then filter on job name, which should be unique for that user
There was a problem hiding this comment.
Done. jobs["do@org.com"]["analysis"]. An @ in the key means datasite, so validate_job_name rejects @ in new job names now, as you suggested, but it handles existing ones. I added a deprecation warning.
Chaining a second email narrows to the ds when a name is duplicated among submitters, so jobs["do@x"]["ds1@y"]["analysis"] disambiguates.
| if not approval_file.exists(): | ||
| print( | ||
| "🟠 Approval file does not exist yet. Kindly wait until enclave sends it." | ||
| raise FileNotFoundError( |
There was a problem hiding this comment.
SyftEnclaveClient.approve_job() now checks the per-party approval file first and raises FileNotFoundError naming the data owner and the job, before job.approve() runs.
Does this pre-check really solve anything?
There was a problem hiding this comment.
Perhaps we could also still write a ticket about this specifically. I think the main problem is that we need for the enclave before we can approve but changing that would mean changing the whole design around enclave jobs
There was a problem hiding this comment.
You're right. It's not really solving anything here. EnclaveJobInfo.approve() is already refusing when the file was missing. The pre-check only changed the message and exception type. Dropped it. Kept Permissionerror as it was before.
| f"Current job is in {self.datasite_owner_email}'s folder." | ||
| f"You are {self.current_user_email}, and job '{self.name}' is on " | ||
| f"{self.datasite_owner_email}'s datasite. Only they can approve " | ||
| f"it. If you meant one of your own, index it by name: " |
There was a problem hiding this comment.
The error raised when JobInfo.approve() is called on a job that sits on a peer's datasite now suggests indexing by name instead.
I don't get it — why would indexing by name always give your own job?
There was a problem hiding this comment.
Correct. With the other correction from above, it now says jobs["<your own email>"]["<name>"]. An email key keeps the jobs it is a party to, so naming yourself keeps your own datasite's jobs and the keys can only resolve there.
| assert do_client.jobs[0].status == "failed" | ||
|
|
||
|
|
||
| def test_jobs_table_hint_uses_name_based_indexing(tmp_path: Path): |
There was a problem hiding this comment.
This file adds ten tests built from JobClient and SyftJobRunner over a tmp_path SyftBox folder; four of them are about the hint under the jobs table.
Perhaps we should isolate the hint tests in their own file.
| # The state a data owner hits when the enclave has not distributed yet. | ||
| approval_file.unlink() | ||
|
|
||
| with pytest.raises(FileNotFoundError) as exc: |
There was a problem hiding this comment.
This deletes the per-party approval file the enclave writes, and asserts SyftEnclaveClient.approve_job() raises FileNotFoundError naming the data owner, the job and client.sync().
A missing approval file shouldn't crash the enclave, and we should double check if thats the case
There was a problem hiding this comment.
Checked. The enclave never calls approve_job, and EnclaveRunner.tick() does not let exceptions propagate anyway.
It can't crash the enclave, but I moved the tests to test_enclave_job_info.py, testing EnclaveJobInfo.approve() so it doesn't imply otherwise.
2 participants
Summary
Make silent job and peer failures visible and actionable
Changes
Testing