fix(cron): read run history from the profile that owns the job - #87936
Open
jackulau wants to merge 1 commit into
Open
fix(cron): read run history from the profile that owns the job#87936jackulau wants to merge 1 commit into
jackulau wants to merge 1 commit into
Conversation
Refs NousResearch#87882 The desktop cron view lists jobs across every profile and then tags its REST calls with whichever profile is currently active, so the two disagree the moment the opened job lives somewhere else. The run-history endpoint treated that request scope as the job's owner and opened a state.db that provably cannot hold a single one of the runs, and the panel said "No runs yet". The mismatch is already detectable at the point it happens: the endpoint looks the job up in the selected profile to canonicalise the id, and gets nothing back. Use that as the signal to find the profile that does own the job and read its store instead. A request whose scope already owns the job keeps exactly the path it had, and a job no profile owns still returns an empty history rather than guessing at a store. Resolving the job also went through cron.jobs.get_job, which compares against the id alone. The call site's own comment says the reference may be a human name, and _find_cron_job_profile matches on one, so a name-referenced job resolved to no record and left the scan looking for run-session ids built from a name that never appears in one. resolve_job_ref accepts either form. A name matching two jobs is reported as unresolved rather than arbitrated, which degrades to the same empty history the ambiguity already produced.
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.
What does this PR do?
GET /api/cron/jobs/{id}/runsreads run sessions out of a profile'sstate.db. It decides which profile that is with:profileis the request scope, not the job's owner, and the desktop is structurally capable of disagreeing about the two: the cron view lists jobs across every profile (_list_cron_jobs_sync("all")) whilegetCronJobRunstags its call with whichever profile is active. Open a job that lives in another profile and the endpoint reads astate.dbthat cannot hold a single one of that job's runs, and the panel says "No runs yet".The mismatch is already detectable exactly where it happens. The endpoint looks the job up in the selected profile to canonicalise the id, and gets nothing back — at which point it knows the scope is wrong and queries anyway. This PR uses that as the signal to find the owning profile and read its store. A request whose scope already owns the job keeps precisely the path it had, and a job no profile owns still returns an empty history rather than guessing at a store.
A second, independent reason the same endpoint returned nothing
Resolution went through
cron.jobs.get_job, which compares against the id alone:The call site's own comment says the reference may be a human name, and
_find_cron_job_profilematches on one, so a name-referenced job resolved to no record and left the scan looking for run-session ids built from a name that never appears in one (they carry the canonical id).resolve_job_refaccepts either form, preferring an exact id match. An ambiguous name is reported as unresolved rather than arbitrated between the jobs it matches.I found this because a test I wrote for the first bug failed for the second reason. It is a separate defect in the same function and the same user-visible symptom; happy to split it out if you would rather review them apart.
Related Issue
Refs #87882
I am deliberately not writing
Fixes, because I cannot show that this is the mechanism the reporter hit, and I would rather say so than let the issue auto-close on a guess. What I can show:state.db-vs-state.dbdiagnosis is right, and their pointer at_list_cron_job_runs_syncis the right function.Two things I checked that are not the cause, so nobody re-treads them:
resolveProfileBackendRoutereturns{backend: 'primary', scopePath: false}for it,startHermes()resolves to the remote descriptor, andpathWithGlobalRemoteProfileleaves the path alone. REST — including this endpoint — reaches the remote gateway and reads its store. For a non-primary profile the same route setsscopePath: trueand appends?profile=, which is one concrete way to land in the bug this PR fixes.HermesApiRequestcarriesprofileand no connection id, andactiveConnection()— the only connection-id-aware resolver on the renderer — is used solely bypluginSocket. So activating a registered remote gateway moves sessions and WS traffic to it while every REST call, the whole cron surface included, stays on the local profile backend. That is an architectural gap, not a one-function bug, and it is not what this PR touches.Type of Change
Changes Made
hermes_cli/web_server.py_list_cron_job_runs_sync: when the selected profile does not own the job, fall back to_find_cron_job_profileand read that profile's store. The fallback runs only on the path that previously returned an empty list, so it costs nothing on a scope that was already correct._resolve_cron_job_in_profile(profile, ref):resolve_job_refin place ofget_job, withAmbiguousJobReferencereported as unresolved.tests/hermes_cli/test_web_server_cron_profiles.py: 6 tests plus two small helpers, built on the existingisolated_profilesfixture. These are end-to-end against real profile homes and a realstate.db— the job is created through_call_cron_for_profile(..., "create_job", ...)and the run session is written withcreate_session/append_message/end_session— not mocks around the function under test.How to Test
Proof each half of the change is load-bearing:
test_run_history_follows_the_owning_profileandtest_run_history_resolves_a_job_named_rather_than_identifiedfail.resolve_job_ref→get_job:test_run_history_resolves_a_job_named_rather_than_identifiedandtest_run_history_resolves_a_name_on_the_unscoped_path_toofail.test_run_history_is_empty_for_an_ambiguous_namepasses either way and is a guardrail, not a proof:get_jobalso returns nothing for a name, so it only pins the behaviour against a future resolver that would pick one of the two matches.test_run_history_reads_the_requested_profile_when_it_owns_the_jobandtest_run_history_is_empty_for_a_job_no_profile_ownsare likewise guardrails on the paths this change must not disturb.Manually, with two profiles:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the affected suites rather than the whole tree; see "Notes on verification"Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AOverlap with #52018 — please read before merging either
#52018 is open, unmerged, and changes the same two files, including this same function. It is a genuinely different bug and the two fixes compose, but they will conflict textually and I would rather flag that than have a maintainer discover it.
jobs.jsonlives in profile A while the scheduler stamped itprofile: B, so the runs are in B's store._annotate_cron_joboverwrites the rawprofilefield before anyone can read it; that PR preserves it asscheduler_profileand opens B.Neither subsumes the other. With #52018's fix alone, a request scoped to C still fails:
get_jobon C returns nothing, so there is no record to readscheduler_profileoff. With this PR alone, their case still fails: A genuinely owns the job, so the fallback never fires. I confirmed onupstream/mainthat_annotate_cron_jobstill drops the field, so their diagnosis stands as written.If #52018 lands first I will rebase onto it; if this lands first their rebase is small (the lookup moves behind
_resolve_cron_job_in_profile). Happy to fold both into one PR if that is easier to review.Notes on verification
pytest tests/hermes_cli/test_web_server_cron_profiles.py -q: 37 passed.pytest tests/hermes_cli/test_cron.py test_web_server_cron_profiles.py test_cron_dashboard_off_loop.py test_cron_fire_dashboard.py test_cron_profile_enumeration_lightweight.py -q: 65 passed.pytest tests/hermes_cli/test_web_server.py -q: 156 passed, 4 skipped.ruff check: all checks passed.ruff format --diffreports one more hunk onweb_server.pythanupstream/maindoes, and it is not a new violation: inserting_resolve_cron_job_in_profilesplit a pre-existing pair of adjacent hunks (the long_list_cron_job_runs_syncsignature and some stray blank lines) into two. The added lines themselves are already formatter-clean.scripts/check-windows-footguns.py --all: no footguns, 973 files scanned.