fix: enforce per-job process isolation - #169
Conversation
pilipilisbot
left a comment
There was a problem hiding this comment.
Thanks for the focused incident fix. I checked the PR metadata/discussion, the full changed diff, the session correlation helpers, queue claim/retry behavior, operations docs, and the targeted mode-dispatch tests. CI is green for the dashboard and both pytest versions. I also attempted a local focused pytest run, but this host lacks pytest in the system interpreter and the throwaway /tmp venv dependency install did not complete in a reasonable time, so I stopped it without changing the worktree.
I agree with the openclaw agent --local direction: keeping the agent and tool tree under the executor worker/service cgroup is the right boundary for this incident.
I do need a change before merge, though: this PR makes session_key_for_rescue(job.work_key, job.id, job.attempts) unconditional for every dispatch. That prevents stale gateway sessions from sharing the old key, but it also permanently turns the OpenClaw session key into a per-job-attempt key. The bridge has been relying on a stable per-work_key session key for conversation isolation across a GitHub thread while using per-job/per-attempt session ids for dashboard correlation. With the current diff, follow-up jobs on the same PR/issue no longer share any OpenClaw conversation key, so every comment/review attempt starts from a fresh conversation even after the gateway-era sessions are gone.
Please preserve a stable per-thread key for normal local dispatches while avoiding the legacy gateway key collision. A durable shape would be something like a new stable local namespace/version for the work key, with the rescue/attempt-scoped key reserved for fresh_session_on_retry or another explicit recovery path. That keeps the cgroup containment fix and avoids recovered gateway races without losing thread continuity for future bridge work.
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved.
I reviewed the dispatch/session changes against the incident described in the PR, the repository concurrency contract, and the operations documentation. The important behavior is now covered in the shadow dispatch tests: OpenClaw is invoked with --local, work_allowed jobs keep fresh per-attempt session ids, and both work_allowed/review_only dispatches use per-attempt session keys so recovered gateway sessions cannot race new local attempts.
I also checked that the existing SQLite work_key coalescing remains the bridge concurrency guard, verified the installed OpenClaw CLI exposes agent --local, and confirmed GitHub Actions are green for dashboard plus pytest on Python 3.11 and 3.12. I could not rerun pytest locally because this inspection environment does not have pytest installed, but the focused changed tests are included in CI and passed there.
|
Rechecked the latest The PR now consistently runs OpenClaw with Verification:
No further owner-level blocker from me on this PR. |
|
Follow-up after the interrupted bridge run: no code changes needed from me on this pass. I re-checked the current PR head Verification:
|
Co-authored-by: pilipilisbot <265267145+pilipilisbot@users.noreply.github.com>
|
Addressed the session-continuity review in Normal local dispatches now use a stable, versioned per- I updated the focused dispatch tests and operations documentation accordingly. |
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved 0f9a54e.
The session-continuity change resolves my earlier blocker: normal local dispatches now use the stable versioned per-work_key key (github-agent-bridge:local-v2:...), while --session-id stays fresh per job/attempt for correlation. Explicit recovery retries marked fresh_session_on_retry still receive the attempt-scoped rescue key, so the legacy gateway collision risk is avoided without losing thread continuity.
Local verification on the PR worktree:
python3 -m compileall -q src tests/test_modes_cli.pygit diff --check 6afb7ad7001685905043c8532f96dd752994e784..HEAD
I could not run pytest locally because this host interpreter still has no pytest module, and GitHub currently reports no checks for the new head.
|
@giscebot resol els conflictes |
Preserve cancellation-aware queue completion together with guarded worker terminal updates. Co-authored-by: ecarreras <294235+ecarreras@users.noreply.github.com>
|
Conflicts with current The only textual conflict was in Validation:
Local pytest remains unavailable because this host has neither the pytest module nor Python |
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved 389bc84 after the conflict-resolution update.
The JobQueue.finish() conflict resolution preserves both required behaviors: cancellation metadata still forces the terminal state to blocked, and expected_locked_by still prevents a late worker completion from overwriting a monitor-blocked job. The new regression test covers the late-worker case.
Verification:
- PR is mergeable again (
mergeable_state=clean) - GitHub Actions are green for
dashboard,pytest (3.11), andpytest (3.12)on389bc84 python3 -m compileall -q src testspassed locallygit diff --check origin/main...HEADpassed locally
Local focused pytest is still unavailable on this host because /usr/bin/python3 has no pytest module.
Summary
Incident
After a gateway restart, OpenClaw recovered bridge sessions independently of
the SQLite queue. Jobs marked
pending,blocked, or evendonecouldcontinue running Destral and other tools under
openclaw-gateway.service,outside the configured two bridge workers.
The previous process monitor also remediated one bad worker by restarting
github-agent-bridge.service, which interrupted the other healthy worker.That service-wide restart is removed.
Isolation model
Each attempt runs as:
github-agent-bridge-job-<job-id>-attempt-<attempt>.scopeThe monitor validates the exact unit name, PID birth time, and cgroup path. On
an explicit ownership mismatch it stops only that validated scope and blocks
only that job. Missing or inconsistent scope metadata produces an alert and no
automatic kill.
Stopping or restarting the executor still stops all bound job scopes, as
expected for operator shutdown, but a job-level fault cannot stop its peer or
the executor.
Normal local dispatches use the versioned
github-agent-bridge:local-v2:...session-key namespace, preserving continuity within a GitHub thread without
colliding with legacy gateway sessions. A retry explicitly marked
fresh_session_on_retryreceives an attempt-scoped rescue key.Validation
python3 -m compileall -q src tests/test_modes_cli.pygit diff --checkactive/runningactive/runningwith the same PID