Skip to content

fix(install): add ownership rows for model-pull-jobs/ and activity.db - #1933

Open
thinmintdev wants to merge 1 commit into
mainfrom
fix/pull-jobs-dir-service-owned
Open

fix(install): add ownership rows for model-pull-jobs/ and activity.db#1933
thinmintdev wants to merge 1 commit into
mainfrom
fix/pull-jobs-dir-service-owned

Conversation

@thinmintdev

Copy link
Copy Markdown
Contributor

⚠️ INSTALLER/PERMISSIONS CHANGE — operator review required

Summary

Fixes #1895: hal0-api (User=hal0) failed every pull-job snapshot write (model.pull_job_persist_failed, WARNING-only fail-soft) and could not read an existing snapshot after a restart, because /var/lib/hal0/model-pull-jobs had no row in the OwnershipStore declarative table (src/hal0/install/perms.py). The installer's root-run brain-model pull (installer/install.sh's bundle-tier auto-pull, before line 2809's doctor perms --fix --force backstop) is the first writer on a fresh install, lazily mkdir-ing the dir root:root 0755 under root's umask — the same O13 birth-ownership bug class as #1546 (hal0.db). No row meant doctor perms --fix had nothing to reconcile.

The issue's "Expected" section also calls out activity.db (+ its -wal/-shm WAL siblings, hal0.activity.AuditStore runs PRAGMA journal_mode=WAL) as missing the identical row — added here too.

  • No install.sh change needed. The bundle-tier brain-model pull (line ~1967) runs well before the doctor perms --fix --force backstop (line 2809), so the table-only fix is sufficient: the backstop now has a declared opinion to reconcile the root-owned dir against.
  • Confirmed runner-image-pull-jobs/ (the sibling runner-image pull store) is NOT affected — it's never touched at install time as root, only ever created by the User=hal0 service itself, so it's born correctly already. Left out of scope.

What changed

src/hal0/install/perms.py:

  • model-pull-jobs/hal0:hal0 2775, glob="*.json" children 0600 (matches tempfile.mkstemp's birth mode, same convention as registry.toml/slots/*/state.json).
  • activity.db / activity.db-wal / activity.db-shmhal0:hal0 0644, mirroring the existing hal0.db rows exactly.

tests/install/test_perms.py:

  • New test_every_hal0_api_write_path_has_an_ownership_row — parametrized over every var_lib() path hal0-api can be the first writer of (the two new ones + hal0.db/registry//slots//models/ as regression anchors), so a future service-writable path can't go unrowed the same way model-pull-jobs/ did. Confirmed RED against main (5 failures) before the perms.py change, GREEN after.
  • New test_model_pull_jobs_dir_snapshot_files_get_hal0_owned_child_rows — asserts the model-pull-jobs/ row's child_mode matches persist_pull_job's actual tempfile.mkstemp birth mode (0600).

Sequencing note

Kept minimal and confined to the missing rows + the coverage test — #1896 (doctor perms never-converges) also touches perms.py in unrelated areas (STATE.md, secrets/, recursive-mkdir modes) and is deliberately held until this lands, so it can rebase cleanly.

Test plan

  • env -u FORCE_COLOR HAL0_HOME=$(mktemp -d) uv run --extra dev pytest tests/install tests/installer -q → 748 passed, 1 skipped
  • Confirmed the 2 new pytest functions are RED on main (5 parametrized failures) before the perms.py fix, GREEN after
  • make lint → clean
  • uv run ruff format --check src tests → clean
  • Operator: re-run rc-validate repro (ls -ld /var/lib/hal0/model-pull-jobs && sudo -u hal0 test -w ...) on a fresh install to confirm the end-to-end fix

🤖 Generated with Claude Code

hal0-api (User=hal0) failed every pull-job snapshot write and could not
read an existing one after a restart, because model-pull-jobs/ had no
row in the OwnershipStore table: the installer's root-run brain-model
pull is the first writer on a fresh install, lazily mkdir-ing the dir
root:root 0755 with no way for `doctor perms --fix` to heal it. The
same O13 birth-ownership class applies to activity.db (+ its -wal/-shm
WAL siblings), which the issue also calls out — no row existed for it
either.

Add a durable coverage test (test_every_hal0_api_write_path_has_an_ownership_row)
parametrized over every var_lib() path hal0-api can be the first writer
of, so a future service-writable path can't go unrowed the same way.

Refs #1895
@thinmintdev

Copy link
Copy Markdown
Contributor Author

Independent review — APPROVE (0 blocking, 4 non-blocking notes)

I did not write this code and verified every claim in the PR body against the tree rather than taking it on trust.

1. Does the diff fix #1895's exact repro?

Yes. Traced the whole path:

  • hal0.registry.pull.persist_pull_job (pull.py:442) does path.parent.mkdir(parents=True, exist_ok=True) then tempfile.mkstemp(dir=path.parent) — lazy, umask-driven birth, exactly as the issue describes.
  • It is reached from run_pull (pull.py:1166, 1662), which the installer's brain-model pull enters as root via hal0.install.brain_modelhal0.install.orchestrate.run_pull_and_activate (brain_model.py:200). So root really is the first writer on a fresh install.
  • install/perms.py had no row for var_lib/"model-pull-jobs", so plan()/commit() had nothing to reconcile — the missing-declaration root cause, not a symptom.

After the change the backstop chowns the dir to hal0:hal0 2775 and any existing *.json snapshot to hal0:hal0 0600, which is precisely what sudo -u hal0 test -w /var/lib/hal0/model-pull-jobs and the post-restart GET /api/models/<id>/pull/status need.

2. Root cause vs symptom

Fix is at the cause (the declarative table is the single source of truth; the O13/#1546 class), not a one-off chown bolted into install.sh. Correct call.

3. Revert-and-confirm-red (independently run, not taken from the PR body)

Detached worktree at 034c1633, git checkout HEAD~1 -- src/hal0/install/perms.py (tests left at PR state):

FAILED tests/install/test_perms.py::test_every_hal0_api_write_path_has_an_ownership_row[activity.db]
FAILED tests/install/test_perms.py::test_every_hal0_api_write_path_has_an_ownership_row[activity.db-wal]
FAILED tests/install/test_perms.py::test_every_hal0_api_write_path_has_an_ownership_row[activity.db-shm]
FAILED tests/install/test_perms.py::test_every_hal0_api_write_path_has_an_ownership_row[model-pull-jobs/]
FAILED tests/install/test_perms.py::test_model_pull_jobs_dir_snapshot_files_get_hal0_owned_child_rows
5 failed, 6 passed, 34 deselected in 0.16s

Exactly the 5 parametrized failures claimed. Restoring perms.py:

  • tests/install259 passed
  • tests/installer489 passed, 1 skipped (shellcheck absent)
  • other perms-touching suites (tests/agents/test_hermes_security_deliverables.py, tests/config/test_loader.py, tests/security/test_api_env_mode.py) → 89 passed
  • ruff format --check src tests1147 files already formatted; ruff check on both changed files → All checks passed!

The tests fail for the right reason (missing table row / KeyError on the row lookup), not incidentally.

4. Modes

  • model-pull-jobs/ 2775 matches the sibling runtime-tree convention (slots/, registry/, models/ are all 0o2775 setgid with state_owner/service_group).
  • child_mode=0o600 for *.json matches the actual birth mode: persist_pull_job uses tempfile.mkstemp, which is always 0600 regardless of umask, then os.replace. Same rationale already documented for registry/registry.toml and slots/*/state.json. Only the daemon reads the snapshots, so 0600 is functionally sufficient too — the row matches the writer instead of fighting it.
  • activity.db{,-wal,-shm} 0644 mirrors the hal0.db trio byte-for-byte, and 0644 is sqlite3.connect's birth mode with a single writer. paths.activity_db() (config/paths.py:184) resolves to var_lib/"activity.db", so the row targets are right.

5. runner-image-pull-jobs/ out of scope

Verified, claim holds. _pull_jobs_dir() for runner images is runner_pull.py:126; its only caller chain is registry/runner_pull_jobs.pyapi/routes/runner_images.py (plus the API lifespan shutdown hook). Nothing in installer/install.sh, the CLI, or any root-run install step touches it, so it is born under User=hal0. Leaving it out is defensible.

6. install.sh needs no edit

Verified the ordering directly:

  • brain-model pull: installer/install.sh:1993 (python -m hal0.install.brain_model), inside the DEV_MODE -eq 0 branch
  • backstop: installer/install.sh:2809 ("${HAL0_BIN}" doctor perms --fix --force), also gated on DEV_MODE -eq 0

Pull strictly precedes the backstop, and both are skipped together in dev mode, so the table-only fix is sufficient. Claim confirmed.

7. Scope discipline

Diff is confined to src/hal0/install/perms.py (rows only, appended in the existing var_lib block) + tests/install/test_perms.py. No install.sh, no STATE.md/secrets//recursive-mkdir churn — #1896 should rebase cleanly.


Non-blocking notes

  1. The "durable coverage" test does not enforce the invariant its docstring claims. _HAL0_API_WRITE_LABELS is a hand-maintained list, so it can only fail for paths someone already remembered to add — i.e. it cannot catch the next unrowed path, which is the exact failure mode it says it prevents. Concretely, these var_lib() paths are written by hal0-api today, have no ownership row, and the new test stays green anyway: update-jobs/ (api/routes/updater.py:221), dashboard-layout.json (dashboard/layout_store.py:36), gpu_arbiter.json (slots/manager.py:3289), stacks/state.json (config/paths.py:490), state/agents/<name>/provision.json, runner-image-pull-jobs/. Suggest either softening the docstring to what it actually is (a regression anchor for known paths) or deriving the list from a real registry of write paths. Not a blocker for installer: root-owned model-pull-jobs dir breaks pull-status persistence for hal0-api across restarts #1895.

  2. optional=False is inert. PermRow.optional is never read by _expand_row, plan, commit, or audit_rows — absent paths are already never changed (PermDiff.changed short-circuits on before.exists) and are reported as a dim absent row. Setting it matches the sibling rows' style, which is fine, but test_model_pull_jobs_dir_snapshot_files_get_hal0_owned_child_rows asserting row.optional is False is asserting a no-op field. Worth knowing before anyone reads the comment as load-bearing.

  3. regressions.yaml entry left untouched. The file header says a repro promoted into pytest should set promoted_to: and drop it in the same PR, and model-pull-jobs-root-owned is promote_ready: true with "Promote: a pytest over the perms table covering every path hal0-api writes" — which is what this PR adds. The new test only covers the table half (not the on-box restart/404 half), so keeping the entry is arguably correct — but that decision should be explicit (either set promoted_to: or say why the entry stays).

  4. Nit, pre-existing: mkstemp temp files are named .<id>.json.XXXX.tmp, so they match neither the new *.json perms glob nor sweep_pull_jobs's *.json glob. A crashed root-run pull can leave a root-owned 0600 stray that nothing heals or reaps. Cosmetic only (the daemon can unlink it once it owns the dir); out of scope here.

Verified with env -u FORCE_COLOR HAL0_HOME=$(mktemp -d) uv run --extra dev pytest ... in an isolated detached worktree at 034c1633.

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.

installer: root-owned model-pull-jobs dir breaks pull-status persistence for hal0-api across restarts

1 participant