diff --git a/CHANGELOG.md b/CHANGELOG.md index 05a4daa..c8b98d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ All notable changes to this project are recorded here. The format follows Keep a ### Fixed +- `doctor` reports an invalid job record without deriving stray-path findings from its unreadable contents. Valid stray and orphan path diagnostics remain available in the same snapshot (related to #75). + - Reject empty or multiline `--acquisition` values in path and semaphore releases. An empty guard previously released the current reservation unconditionally; invalid guards now return exit 2 before changing any reservation (related to #74). - Reject NUL bytes in batch input before parsing or publication. Stored blobs and failed Git batch diagnostics with NUL bytes fail with structured JSON errors instead of losing bytes or leaking Bash warnings. Batch input uses a direct shell read, so signaling the CLI leaves no input-reader child or named capture file. Private snapshot capture files are removed after success or failure; wrapped-command binary streams remain unchanged. diff --git a/Makefile b/Makefile index 0b33e6b..97503d6 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,7 @@ test-container: python3 test/store-environment.py python3 test/store-selection.py python3 test/store-integrity.py + python3 test/doctor-findings.py python3 test/renewal.py python3 test/release-guards.py python3 test/wrapper-lifecycle.py diff --git a/bin/git-locks b/bin/git-locks index bc2ade0..6dc7867 100755 --- a/bin/git-locks +++ b/bin/git-locks @@ -3303,7 +3303,7 @@ cmd_doctor() { ensure_snapshot local rows ref oid job name rest at refs_n=0 recs_n="${#BLOB[@]}" hook_policy doctor_hook_policy hook_policy - local -A JOB_OID=() OID_JOBS=() PATHREF_OID=() EXPECTED_PATHREF=() JOB_OK=() + local -A JOB_OID=() OID_JOBS=() PATHREF_OID=() EXPECTED_PATHREF=() JOB_OK=() LOCK_OK=() local -A SEM_META=() SEM_GEN=() SEM_SLOT_OIDS=() SEM_SLOT_JOBS=() SEM_NAMES=() local jobs=() sems=() pathrefs=() all_paths=() p paths now_v at @@ -3350,6 +3350,7 @@ cmd_doctor() { oid="${JOB_OID[${job}]}" doctor_lock_record "${job}" "${oid}" || continue JOB_OK["${job}"]=1 + LOCK_OK["${oid}"]=1 field_v rest "${oid}" job [[ "${rest}" == "${job}" ]] || finding job-ref-name "${job}" "the job ref points at a record for job '${rest}'" record_paths_v paths "${oid}" @@ -3385,7 +3386,7 @@ cmd_doctor() { oid="${PATHREF_OID[${ref}]}" if [[ -z "${OID_JOBS[${oid}]+x}" ]]; then finding path-ref-orphan "${ref}" "points at record ${oid}, which no job ref points at: the path reads as held by nothing a release can name" - elif key="${ref}|${oid}" && [[ -z "${EXPECTED_PATHREF[${key}]+x}" ]]; then + elif [[ -n "${LOCK_OK[${oid}]+x}" ]] && key="${ref}|${oid}" && [[ -z "${EXPECTED_PATHREF[${key}]+x}" ]]; then finding path-ref-stray "${ref}" "points at record ${oid} (job ${OID_JOBS[${oid}]% }) which lists no path hashing to this ref" fi done diff --git a/docs/usage.md b/docs/usage.md index 5266b32..e6decdd 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -19,7 +19,7 @@ Run commands from the subject repository's root. Path keys are lexical, repo-rel Paths must be valid UTF-8. They can contain spaces but cannot contain newlines. Case, Unicode composition, symlinks, and hard links do not normalize to one key. All workers must agree on names, even when two names address the same file. -Holder names, notes, acquisition guards, batch input, and the selected store pathname must also be valid UTF-8. Malformed text is refused with exit 2 before any reservation is published. Existing authority records with malformed UTF-8 block ordinary operations; `doctor` reports the invalid records without changing them. Error output replaces each invalid diagnostic byte with U+FFFD, so external errors still form valid JSON. Reservation keys are never repaired or replaced. A wrapped program keeps control of its own arguments and output. +Holder names, notes, acquisition guards, batch input, and the selected store pathname must also be valid UTF-8. Malformed text is refused with exit 2 before any reservation is published. Existing authority records with malformed UTF-8 block ordinary operations; `doctor` reports the invalid records without changing them. It reports a stray path index only when the owning record can be decoded; an unreadable path list is not evidence of a stray entry. Error output replaces each invalid diagnostic byte with U+FFFD, so external errors still form valid JSON. Reservation keys are never repaired or replaced. A wrapped program keeps control of its own arguments and output. Batch input must not contain NUL bytes. A bad batch fails with exit 2 before any reservation is written. Stored records with NUL bytes block all state reads, including `doctor`, with a structured `store-read` error. The CLI never removes NUL bytes to make a record valid. Wrapped programs retain control of their binary input and output. diff --git a/lib/175-doctor.sh b/lib/175-doctor.sh index 1be7629..677bf34 100644 --- a/lib/175-doctor.sh +++ b/lib/175-doctor.sh @@ -78,7 +78,7 @@ cmd_doctor() { ensure_snapshot local rows ref oid job name rest at refs_n=0 recs_n="${#BLOB[@]}" hook_policy doctor_hook_policy hook_policy - local -A JOB_OID=() OID_JOBS=() PATHREF_OID=() EXPECTED_PATHREF=() JOB_OK=() + local -A JOB_OID=() OID_JOBS=() PATHREF_OID=() EXPECTED_PATHREF=() JOB_OK=() LOCK_OK=() local -A SEM_META=() SEM_GEN=() SEM_SLOT_OIDS=() SEM_SLOT_JOBS=() SEM_NAMES=() local jobs=() sems=() pathrefs=() all_paths=() p paths now_v at @@ -125,6 +125,7 @@ cmd_doctor() { oid="${JOB_OID[${job}]}" doctor_lock_record "${job}" "${oid}" || continue JOB_OK["${job}"]=1 + LOCK_OK["${oid}"]=1 field_v rest "${oid}" job [[ "${rest}" == "${job}" ]] || finding job-ref-name "${job}" "the job ref points at a record for job '${rest}'" record_paths_v paths "${oid}" @@ -160,7 +161,7 @@ cmd_doctor() { oid="${PATHREF_OID[${ref}]}" if [[ -z "${OID_JOBS[${oid}]+x}" ]]; then finding path-ref-orphan "${ref}" "points at record ${oid}, which no job ref points at: the path reads as held by nothing a release can name" - elif key="${ref}|${oid}" && [[ -z "${EXPECTED_PATHREF[${key}]+x}" ]]; then + elif [[ -n "${LOCK_OK[${oid}]+x}" ]] && key="${ref}|${oid}" && [[ -z "${EXPECTED_PATHREF[${key}]+x}" ]]; then finding path-ref-stray "${ref}" "points at record ${oid} (job ${OID_JOBS[${oid}]% }) which lists no path hashing to this ref" fi done diff --git a/test/doctor-findings.py b/test/doctor-findings.py new file mode 100644 index 0000000..4667816 --- /dev/null +++ b/test/doctor-findings.py @@ -0,0 +1,120 @@ +#!/usr/bin/env python3 +"""Doctor must distinguish an unreadable record from a proven index defect.""" +from pathlib import Path +import subprocess + +ROOT = Path(__file__).resolve().parents[1] +subprocess.run(['node', str(ROOT / 'scripts/require-docker.mjs')], check=True) + +import json +import os +import tempfile +from collections import Counter + +import jsonschema + +CLI = str(ROOT / 'bin/git-locks') +BASE = {k: v for k, v in os.environ.items() if not k.startswith('GIT_')} +VALIDATOR = jsonschema.Draft202012Validator(json.loads((ROOT / 'schema/git-locks.schema.json').read_text())) + + +def git(store, *args, data=None): + return subprocess.check_output(['git', '--git-dir=' + str(store), *args], input=data, + env=BASE, timeout=10).strip() + + +def publish(store, entries): + # Build the fixture independently of the CLI's private-index publisher. + root = {} + for path, oid in entries.items(): + node = root + parts = path.split('/') + for part in parts[:-1]: + node = node.setdefault(part, {}) + node[parts[-1]] = oid + + def tree(node): + rows = [] + for name, value in sorted(node.items()): + if isinstance(value, dict): + rows.append(b'040000 tree ' + tree(value) + b'\t' + name.encode() + b'\n') + else: + rows.append(b'100644 blob ' + value + b'\t' + name.encode() + b'\n') + return git(store, 'mktree', data=b''.join(rows)) + + oid = tree(root) + git(store, 'update-ref', 'refs/locks/state', oid.decode()) + return oid + + +def exercise(base, mode): + store = base / 'store.git' + env = dict(BASE, GIT_LOCKS_STORE=str(store)) + for job in ('a', 'b'): + result = subprocess.run([CLI, 'claim', '--job', job, '--holder', 'alice', + '--ttl', '300', job + '.md'], env=env, capture_output=True, timeout=10) + assert result.returncode == 0, result + entries = {line.split(b'\t')[1].decode(): line.split()[2] + for line in git(store, 'ls-tree', '-r', 'refs/locks/state').splitlines()} + a_oid = entries['jobs/a'] + a_record = git(store, 'cat-file', 'blob', a_oid.decode()) + b'\n' + expected = [] + if mode in ('malformed', 'utf8', 'no-paths', 'bad-time', 'bad-alias', 'mixed'): + if mode == 'utf8': + damaged = a_record.replace(b'holder: alice', b'holder: \xff') + elif mode == 'no-paths': + damaged = a_record[:a_record.index(b'paths:\n')] + b'paths:\n' + elif mode == 'bad-time': + damaged = a_record.replace(b'expires: ', b'expires: invalid') + else: + damaged = b'not a lock record\n' + bad_oid = git(store, 'hash-object', '-w', '--stdin', data=damaged) + entries = {key: bad_oid if value == a_oid else value for key, value in entries.items()} + expected.append(('record-decodes', 'a')) + if mode == 'bad-alias': + entries['jobs/alias'] = bad_oid + expected.append(('record-decodes', 'alias')) + if mode in ('stray', 'mixed'): + key = 'paths/' + git(store, 'hash-object', '--stdin', data=b'unlisted.md').decode() + entries[key] = entries['jobs/b'] + expected.append(('path-ref-stray', 'refs/locks/' + key)) + if mode in ('orphan', 'mixed'): + if mode == 'orphan': + orphan_oid = entries.pop('jobs/a') + key = 'paths/' + git(store, 'hash-object', '--stdin', data=b'a.md').decode() + else: + orphan_oid = git(store, 'hash-object', '-w', '--stdin', data=a_record.replace(b'job: a', b'job: gone')) + key = 'paths/' + git(store, 'hash-object', '--stdin', data=b'orphan.md').decode() + entries[key] = orphan_oid + expected.append(('path-ref-orphan', 'refs/locks/' + key)) + if mode == 'valid-alias': + entries['jobs/alias'] = a_oid + expected.append(('job-ref-name', 'alias')) + before = publish(store, entries) + objects = git(store, 'cat-file', '--batch-all-objects', '--batch-check=%(objectname)') + result = subprocess.run([CLI, 'doctor'], env=env, capture_output=True, timeout=10) + assert result.returncode == int(bool(expected)) and not result.stderr, result + rows = [json.loads(line) for line in result.stdout.decode('utf-8', errors='strict').splitlines()] + for row in rows: + VALIDATOR.validate(row) + findings = rows[:-1] + actual = Counter((row['check'], row['subject']) for row in findings) + assert actual == Counter(expected), (mode, actual, expected) + assert rows[-1]['findings'] == len(expected) and rows[-1]['healthy'] == (not expected), rows[-1] + assert git(store, 'rev-parse', 'refs/locks/state') == before, 'doctor changed the root' + assert git(store, 'cat-file', '--batch-all-objects', '--batch-check=%(objectname)') == objects, 'doctor wrote objects' + + +failures = [] +modes = ('healthy', 'malformed', 'utf8', 'no-paths', 'bad-time', 'bad-alias', + 'stray', 'orphan', 'mixed', 'valid-alias') +for mode in modes: + with tempfile.TemporaryDirectory(prefix='locks-doctor-') as tmp: + try: + exercise(Path(tmp), mode) + print('PASS', mode, flush=True) + except Exception as error: + failures.append((mode, repr(error))) + print('FAIL', mode, repr(error), flush=True) +print(f'doctor findings: {len(modes) - len(failures)} passed; {len(failures)} failed') +raise SystemExit(bool(failures))