Skip to content

fix(paseo-kit): reconcile pre-boot daemon lock - #488

Open
mogul wants to merge 3 commits into
mainfrom
fix/paseo-daemon-lock-reconciliation-487
Open

mogul wants to merge 3 commits into
mainfrom
fix/paseo-daemon-lock-reconciliation-487

Conversation

@mogul

@mogul mogul commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Context

Fixes #487. A restored or migrated sandbox can retain a healthy Paseo process tree while paseo.pid has a startedAt timestamp from before the current kernel boot. Local CLI discovery then rejects the lock and reports the daemon as stopped.

Changes

  • Reconcile pre-boot locks during startup only.
  • Require the lock PID to be the same-user Paseo supervisor, recheck process identity before signaling, and wait for the captured process tree plus lock release.
  • Leave post-boot, malformed, ambiguous, dead, or unreconciled locks untouched and log the reason.
  • Add regression coverage for pre-boot, post-boot, malformed, and ambiguous lock cases.
  • Extend the live verifier to assert default local paseo daemon status and paseo ls discovery while retaining explicit-host reachability checks.
  • Update the Paseo 0.9 decision record for restored/migrated sandboxes.

Verification

  • node --test integrations/isolation/acq-kits/paseo/tests/**/*.test.mjs passed: 16 tests.
  • sh -n integrations/isolation/acq-kits/paseo/files/home/paseo-start.sh passed.
  • bash -n integrations/isolation/acq-kits/paseo/scripts/verify passed.
  • git diff --check passed.
  • make test-kits passed: all usai-provider, Paseo, and providers/usai tests.
  • make validate and make scan-shell were blocked because the environment has no python executable.
  • python3 integrations/isolation/acq-kits/validate-kits.py --strict was blocked because python3 lacks jsonschema.
  • Live sandbox validation was not run to avoid disrupting the running Paseo process.

Rollback

Revert commit fd5fef8.

Security Impact

No dependency, authentication, authorization, network, or CI changes. Startup reconciliation is fail-closed and does not manually unlink locks; it signals only a verified same-user Paseo supervisor with a pre-boot lock and uses a bounded graceful path.

AI Disclosure

This change was implemented with assistance from OpenCode. Human review is required before production deployment.

Reconcile a live Paseo supervisor whose lock predates the current kernel boot during startup, then verify default local CLI discovery.

Co-authored-by: OpenCode Agent <user@gsa.gov>
@mogul
mogul requested a review from a team as a code owner October 8, 2026 07:20
@wz-gsa

wz-gsa commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head fd5fef82.

Ran the suite: 16/16 pass, matching your count. The four new reconciliation cases (pre-boot, post-boot, malformed, ambiguous) all execute.

The identity gating is the right shape and tighter than "is the PID alive". Before signalling, it requires: lock timestamp strictly older than /proc/stat btime; cmdline matching supervisor-entrypoint.js; Uid: equal to id -u; readable starttime. Then it re-reads cmdline and start-ticks and compares them to the pre-check values immediately before kill -TERM, which closes the PID-reuse window between check and signal — that is the part most implementations skip. Every refusal path logs a distinct reason and returns non-zero, and the caller treats non-zero as "do not start a second daemon", so an unreconciled lock fails closed rather than racing.

Also checked the test hook, since a test-only branch in a production startup script that overrides /proc root and kernel boot time is worth confirming is unreachable in a sandbox: PASEO_START_RECONCILE_TEST / PASEO_PROC_ROOT / PASEO_KERNEL_BOOT_TIME appear only in paseo-start.sh and the test file — not in spec.yaml, not in any other kit file. Nothing in the kit's env path sets them.

Two observations on the lock parser, neither blocking

paseo.pid is guest-writable, so I probed the Node parser with 16 inputs. The pid validation is solid — pid <= 1, floats, negatives, array roots, and malformed JSON all exit 2, and __proto__ in the lock does not pollute. Two inputs pass that arguably should not:

{"pid":1234,"startedAt":-1}              -> 1234  -1
{"pid":1234,"startedAt":99999999999999}  -> 1234  99999999999999
{"pid":"1234","startedAt":...}           -> 1234  (string coerced)
  • A negative or absurd startedAt survives Number.isFinite. -1 is always < btime, so it makes a lock maximally eligible for reconciliation; the far-future value makes it permanently ineligible. Neither is exploitable on its own, because the cmdline/uid/start-ticks gate still has to agree — but startedAt is the field that decides whether to try at all, and it is the one field a guest-writable file fully controls. A epoch > 0 && epoch <= now bound would make the eligibility decision structural rather than relying on the later gate to catch it.
  • pid as a string is accepted via Number() coercion. Harmless here, since the value is re-validated as *[!0-9]* in shell and then used as a /proc path, but it means the JSON contract is looser than the comment ("rejects extra/malformed fields") implies.

One note on the 20-second wait

The post-TERM loop waits up to 20s for both the captured tree to exit and the lock file to disappear, then times out and leaves the lock untouched. That is the correct fail-closed direction. Worth being aware that the tree snapshot is taken once, before the signal: a child spawned after the walk but before TERM would not be in _tree_pids, so the loop could see an empty tree while that child is still alive. The [ ! -f "$_lock" ] conjunct is what actually covers this, since Paseo releases the lock on supervisor exit — so the behaviour is right, but it rests on the lock-release half rather than the tree half. Fine as-is; noting it because the tree walk reads like the primary guard and isn't.


Method: ran node --test on the kit's four test files (16/16); transcribed the lock-JSON parser and probed 16 inputs including negative/huge/Infinity startedAt, string and float pid, __proto__, and malformed JSON; grepped both repos for the three test-hook variables to confirm they are unreachable from kit config.

UNVERIFIED: no live sandbox here, so the actual restore/migration scenario, paseo daemon status / paseo ls discovery, and the verifier's 13 new lines are taken on your evidence. You note make validate and scan-shell were blocked in your environment by a missing python/jsonschema — I did not run those either, so neither of us has that signal on this head.

@wz-gsa wz-gsa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting my head-pinned advisory review into a formal approval. Current head is still fd5fef82; no commits or author comments landed after that review, all seven checks pass, and the PR is mergeable. I ran the kit suite at this exact head (16/16), verified the pre-signal identity checks and PID-reuse re-check, and confirmed the test-only overrides are unreachable from kit configuration. The startedAt range and string-pid observations in my earlier comment remain non-blocking hardening suggestions, not correctness findings. AI-assisted review (OpenCode), approved with the maintainer’s explicit authorization; a human owns the merge.

@wz-gsa wz-gsa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correcting the formal review state: the head is unchanged at fd5fef8 and the guest-controlled bound from my 2026-10-08 review remains unresolved. and absurd future epochs pass ; the former makes a lock maximally eligible for reconciliation and the latter permanently ineligible. The cmdline/uid/start-ticks gate prevents direct signalling abuse, so this is not an exposure blocker, but is the field deciding whether reconciliation is attempted. Please bound it (, or equivalent) or explicitly document/accept the trust decision before merge. The later approval under my account came from a concurrent review that ran the 16 tests but did not address this finding.

Reject implausible lock timestamps and non-numeric PIDs before reconciliation.

Co-authored-by: OpenCode Agent <user@gsa.gov>
@mogul

mogul commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the requested lock-metadata validation in bdb0c5a.

  • Reject startedAt values that are non-positive or in the future before deciding whether reconciliation is eligible.
  • Require pid to be a JSON integer rather than coercing strings.
  • Added regression coverage for negative and future timestamps plus string PIDs; each remains untouched and logs as malformed/ambiguous.

Verification:

  • node --test integrations/isolation/acq-kits/paseo/tests/**/*.test.mjs (20 passed)
  • make test-kits (passed)
  • sh -n integrations/isolation/acq-kits/paseo/files/home/paseo-start.sh (passed)
  • bash -n integrations/isolation/acq-kits/paseo/scripts/verify (passed)
  • git diff --check (passed)
  • make validate and make scan-shell remain blocked in this environment because python is unavailable.

Could you please re-review the updated head?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(paseo-kit): reconcile pre-boot daemon lock at startup

2 participants