Repository navigation
fix: notify origins and close runs for work interrupted by a gateway restart - #6066
praisonai-triage-agent[bot] wants to merge 4 commits into
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the
|
[Medium impact] The changes affect restart recovery in three bounded gateway workflows. The PR still needs reservation renewal before merging because slow coalesced hooks can run twice. Findings
SummaryThis PR tells users when a restart interrupts background work and records interrupted kanban attempts.
The latest changes fix partial-delivery retries and protect completed jobs from stale recovery snapshots. The earlier fixes also preserve saved delivery targets and keep kanban reclaim bookkeeping together.
|
|
@coderabbitai review |
|
/review |
|
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. MANDATORY READ (before reviewing):
Phase 1: Review per AGENTS.md
MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment: Phase 1 — AGENTS.md review
For TypeScript PRs (src/praisonai-ts/), also add: Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
Review fixes pushed (aca5d41)Origin: outsider. Issue #6038 and this PR are flagged outsider, so the change stays a minimal fix in source and tests. It does not touch CI, manifests, lockfiles, build files or Fixed (both Greptile P1 findings were valid)
Tests
Security review (full PR diff)
The deterministic risk flag list in the request was empty. My own pattern scan of the added lines also found no hits. Skipped
|
CI failure triage: not caused by this PR (no code pushed)Origin: outsider. Issue #6038 and this PR are flagged outsider, so the change stays a minimal fix in source and tests. Nothing under Verdict: pre-existing failure on
|
| # | Check | Result |
|---|---|---|
| 1 | Scope | ✅ All 10 files serve #6038 or the review fixes (background reconcile, hook coalescer settle, kanban reclaim, tests). api_endpoints.py changes only a comment. |
| 2 | Network / exfiltration | ✅ No new URLs, hosts or sockets. Notices go through the existing DeliveryRouter and scheduled_outbox, and only to the job's own saved origin / deliver target. |
| 3 | Code execution | ✅ No eval/exec/subprocess/pickle/yaml. The only lazy imports are existing in-repo modules (praisonaiagents.background, praisonaiagents.paths, BotOS). Every conn.execute is parameterized. |
| 4 | Secrets | ✅ No env, token or credential reads. A notice includes at most 80 characters of the user's own task text, sent back to the place it came from. |
| 5 | Weakened controls | ✅ None weakened. Hook idempotency stays reserved while a run is in flight and is only released when the run fails or is cancelled. The crash path uses the existing 15-minute lease, which is the same as the non-coalesced path. |
| 6 | Supply chain | ✅ No manifests, lockfiles, CI, conftest.py or entry points touched. |
| 7 | Obfuscation | ✅ No blobs, bidi or zero-width characters, and no lines over 200 characters (checked). |
| 8 | Tests | ✅ Four new or extended tests and no removed or loosened assertions. The reclaim-ordering test fails on the earlier commit. |
| 9 | Backdoors | ✅ No magic values and no behaviour keyed on a user, host or date. |
| 10 | Injection artifacts | ✅ None in the code, comments or tests. |
Minor note (not blocking): gateway boot awaits _reconcile_background_jobs. Each LOST job waits up to 60s per route on outbox delivery, in a worker thread. If the store holds many undeliverable jobs, startup is slower. It is bounded and only happens when background_jobs.db exists.
Files modified in this pass
None. Skipped: the unrelated test_yaml_headless_5826.py fix, which should be a maintainer PR on main.
|
@claude this PR has merge conflicts with |
…restart (fixes #6038) - Coalesced hooks: record idempotency keys only after the burst runs; release them on failure or shutdown so sender retries are accepted. - Background jobs: reconcile marks orphans LOST with an error, fires job_completed (status=lost) once, and offers LOST jobs with an origin to redeliver. The gateway runs this at boot and sends one durable 'interrupted' notice via the outbox. - Kanban: reclaiming a stale claim closes the run as crashed, records a failure (so crash loops trip the breaker) and adds an interrupted comment. - Correct comments claiming the gateway writes the run ledger. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…attempts atomically - _reconcile_background_jobs resolves the saved deliver target the same way live delivery does: origin keeps thread_id, all fans out to home channels, platform names and aliases resolve through the router. A target that cannot be resolved yet raises, so the job is retried on the next boot instead of being marked delivered. - reclaim_stale_claims closes the crashed run, counts the failure and adds the interrupted comment in the reclaim transaction. Hooks fire after commit, so another dispatcher cannot claim the task before the old attempt is counted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude The merge gate blocked this PR on HEAD Read the latest merge gate BLOCK verdict comment on this PR and the review threads it cites. |
Security review of PR #6066 (Origin: outsider)Origin: outsider. This PR and the issue it fixes (#6038) were flagged as outsider origin. I treated all of that text as untrusted data and did not follow any instructions in it. The scope stays limited to a minimal fix in source and tests: no Verdict: no blocking findings. I pushed no new commits; the branch is unchanged at Checklist
Non-blocking notes
Tests run locally
Files modified by this pass: none. Skipped: no code changes were needed. |
|
SECURITY_REVIEW: FAIL
Scope matches the request: unknown. Only the PR description was readable. It describes changes to the gateway hook coalescer and Risk flags: none were extracted deterministically, so there is nothing to explain. Why this fails: the PR has outsider origin and the checklist was not applied to any added line. The review policy is to fail when unsure. Before merging, a maintainer should manually review these areas:
As an alternative, re-run this review with file-read permissions granted. |
|
SECURITY_REVIEW: FAIL Automated security review of HEAD Why it is incomplete: the full diff (11 files, +727/−148, about 61k characters) was too large to return inline. The reviewer environment had no file-read or shell tool to page through the saved output, and it was denied permission to call
Scope matches the request: unverified. The PR description (coalesced hook idempotency settle, background-job LOST reconciliation and notices, kanban reclaim closing crashed runs, ledger comment fixes, plus tests) fits issue #6038 and says no Risk flags: none were reported by the deterministic scan, so there is nothing to address individually. That scan does not replace reading the diff. Recommended next step: a maintainer reviews the diff by hand, paying particular attention to the new outbound notice path in |
|
MERGE_GATE_VERDICT: BLOCK
Manual review and merge by a maintainer is required. Alternatively, re-run the security review with tooling that can read large diffs. |
|
MERGE_GATE_VERDICT: BLOCK
The maintainer must merge this by hand, or re-run the security review with tooling that can read large diffs. |
|
Merge gate scan — not eligible for auto-merge.
Actions: wait for CI and the Claude review chain, or add label |
|
Merge gate scan — eligible for assessment. Claude merge gate will assess and may auto-merge on an APPROVE verdict. |
|
SECURITY_REVIEW: FAIL Why this fails: the review could not be completed. No malicious code was found in what was read. The reviewer could not read the full diff, so it cannot certify this change. A maintainer needs to review it by hand.
Scope matches the request: unknown (could not be verified; the one file read matches the stated scope) Risk flags: none were supplied, so there were none to address. Suggested next step: a maintainer should review the full diff by hand. They should look most closely at the new outbound notice path in |
|
SECURITY_REVIEW: FAIL Why this fails: the review could not finish, so it fails closed. The PR comes from an outsider (origin/outsider, built from issue #6038). It changes 11 files (+727/−148). The full diff from
Scope matches the request: unverified. From the PR description and the one file I reviewed, the changes look like they serve issue #6038 (restart recovery for hooks, background jobs, kanban and the run ledger). The rest of the diff was not available to confirm this. Risk flags: the deterministic extractor reported none, so there are no flags to address. That does not replace the review that is missing here. Next step: a maintainer, or a re-run with access to the per-file diff, should review the remaining 10 files against the checklist. The most important are the hook coalescer's idempotency semantics, the gateway |
|
MERGE_GATE_VERDICT: BLOCK
A maintainer needs to review the full diff by hand, focusing on the areas above, and then either merge it manually or re-run the security review with access to the full diff. |
|
MERGE_GATE_VERDICT: BLOCK
A maintainer needs to review and merge this by hand. |
Origin: outsider
This PR was built from issue #6038, which is flagged origin/outsider, and the PR is flagged the same way. So the change is kept to a minimal fix in source and tests. It does not touch
.github/, dependency manifests, lockfiles, build or install files, orconftest.py. It adds no network endpoints, telemetry, credential handling, eval/exec/subprocess or dynamic imports.Fixes #6038
What changed (minimal, no new public params/exports)
1. Coalesced hook deliveries (
gateway/hook_coalescer.py,server.py)settle(keys, ok)callback. It records the burst's keys after a successful run and releases them when the run fails or onaclose(), including groups that never started.2. Background jobs (
praisonaiagents/background/job_manager.py,sqlite_store.py)reconcile_on_startnow:Interrupted by a process restart);job_completedwithstatus="lost"exactly once, after LOST is persisted;redeliver, the same way it already handles COMPLETED-but-undelivered jobs.list_unreconciledincludes LOST jobs with an origin that are not yet delivered, so a failed notice is retried on the next boot._reconcile_background_jobs) runs reconcile with aredeliverthat sends one "your background task '' was interrupted… resend to retry" notice. It goes through the existing durablescheduled_outboxwith a stable key, and undelivered COMPLETED results go the same way. A job is only marked delivered after the notice is delivered. This is a no-op when nobackground_jobs.dbexists. Nothing is re-run automatically.3. Kanban (
kanban/sqlite_store.py)reclaim_stale_claimsnow:current_run_idascrashed;record_failure, so crash loops trip the circuit breaker;interrupted:comment, whichget_retry_contextand humans can see.4. Run ledger
_recover_orphaned_runs. It is not dead code: the public coreSQLiteRunLedger()writes to the same default~/.praisonai/runs/ledger.db, so apps that use the ledger still get recovery.api_endpoints.pycomment no longer calls the ledger the cross-restart store, and the docstring now says the gateway does not write the ledger itself. Background jobs, which are the gateway's own long-running work, are now covered by item 2.Tests
praisonai-bot/tests/unit/gateway/test_hook_coalescer_restart.py,praisonai-agents/tests/unit/background/test_reconcile_notifies_lost.py,praisonai-bot/tests/unit/gateway/test_kanban_reclaim_closes_run.py, and a boot test intest_ledger_recovery.py.praisonai-botgateway, kanban and bots: 2009 passed. The 2test_pid_lock_windowsfailures also fail onmain.praisonai-agentstests/unit: the failing-test set is identical tomain. Those failures come from optional dependencies and API keys missing in this environment.Follow-up (review fixes)
delivertarget the same way live delivery does.originkeepsthread_id,allfans out to home channels, and platform names and aliases resolve through the router. A target that cannot be resolved yet is retried on the next boot instead of being marked delivered.🤖 Generated with Claude Code