Do not let an unreadable job result JSON kill the praktika runner before it reports

[groeneai]

Do not let an unreadable job result JSON kill the praktika runner before it reports

Changelog category (leave one):

• CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Not user-facing: CI harness only.

Description

A Stateless tests (amd_llvm_coverage, AsyncInsert, s3 storage, parallel) job whose 11882 tests
all passed was reported as a red node with a completely empty info - a blank red box in the
report, no artifacts uploaded, and the dependent LLVM Coverage job plus Mergeable Check reddened
with it. CIDB records check_status='error', test_name='' and zero failing test rows.

Observed on
this job
(report),
and 39 times on that shard alone over the last 21 days across 38 unrelated PRs (26 more on
ParallelReplicas, s3 storage, parallel, 13 on old analyzer, ... 2/3, plus ~20 singletons on the
other coverage shards). It concentrates on the longest coverage jobs, i.e. the ones with the
heaviest post-run .profraw merge.

Root cause

The job finished all tests, then spent ~4 minutes in post-processing (jemalloc flamegraphs, then
llvm-profdata merge of 22 x 176 MB .profraw files). Mid-merge the host docker daemon went away:

level=error msg="Error waiting for container: Canceled: grpc: the client connection is closing: context canceled"
--- Fixing file ownership after running docker as root
error during connect: Get "http://%2Fvar%2Frun%2Fdocker.sock/_ping": EOF

Serializable.dump is open(self.file_name(), "w") + json.dump, and open(..., "w") truncates
the target to zero bytes before the first byte of the payload is written. A process killed inside
that window therefore leaves a 0-byte result file, destroying the RUNNING result _pre_run had
persisted. The from-root chown whose entire purpose is to make that file host-readable failed too
(the daemon was gone), and its Shell.run return value is discarded.

Result.from_fs then raises JSONDecodeError: Expecting value: line 1 column 1 (char 0). There are
two job-result reads in Runner.run:

• runner.py:490, inside with TeePopen(...) as process:. Its raise unwinds into run's try
  (body 1071..1086, except Exception at 1087..1091) and is caught - the first traceback in
  the log carries praktika's own [timestamp] prefix because traceback.print_exc() printed it.
• runner.py:1093, which is not guarded, while its sibling at :548 already is. Its raise is
  the interpreter's own unhandled-exception dump (second traceback, no timestamp prefix, via
  runpy -> __main__.py:393), immediately followed by
  ##[error]Process completed with exit code 1.

So the runner dies at :1093, before _get_result_object, _post_run, the html report hook, the
S3 upload and the commit status - grep of the job log finds === Run script finished,
=== Post run script and Run html report hook zero times. Nothing ever attaches a reason, so
_finish_workflow (native_jobs.py:1049-1083) later finds the job result not is_completed(),
stamps ERROR and records NOT_FINALIZED. That is exactly why info is empty.

The fix

1. Make Result.dump() atomic (ci/praktika/result.py). Write to a sibling temp file,
flush + fsync, then os.replace() onto the target, removing the temp on any failure. A reader
now always sees either the previous complete content or the new complete content. Result inherits
dump() and defines none of its own on master, so this single override covers every Result write
path - all of complete_job, copy_result_to_s3[_with_version] and the _dump_if_persisted setter
path included. The file content is byte-identical to the base implementation - only the write is
staged. The temp must be a sibling: os.replace is atomic only within one filesystem.

Scoped deliberately to Result. MetaClasses.Serializable.dump and SerializableSingleton.dump are
the same in-place open(..., "w") pattern, so _Environment, RunConfig, TestCaseIssueCatalog,
StorageUsage and ComputeUsage can lose their files the same way. None of them is the file that
caused this incident, and losing a usage counter degrades a metric rather than blanking a job report,
so widening the change is left for whoever wants it rather than bundled here.

This also fixes the write half of the same infra event, which is easy to miss. When the from-root
chown fails the result file is left root-owned, and the base open(target, "w") then raises
PermissionError on the host side; os.replace needs write permission on the directory, not on
the target, so the host-side dump at runner.py:510 succeeds where an in-place write could not.

The temp path is derived from the result path and this pid, so it is guessable by anything sharing
the worktree. The staged write therefore uses
os.open(tmp, O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW, 0o666) rather than open(tmp, "w"): a
pre-existing symlink at the temp path is refused with ELOOP instead of being written through.
Two details are deliberate. The mode is 0o666 so that, after umask, the created file's mode is
byte-identical to what open(path, "w") produces (0o664 under the CI umask) - tempfile.mkstemp()
would silently narrow the result file to 0o600 for every downstream reader, and its random name
would also give up the per-pid determinism. And the flags do not include O_EXCL: a leftover
temp from a previously killed dump must stay reusable, otherwise the very crash this method exists
to survive would turn into a hard FileExistsError. Both properties are pinned by tests.

2. Recover at the _run read (ci/praktika/runner.py). A new
Runner._read_job_result_or_running returns a RUNNING result when the file is unreadable, so
control enters the existing if not result.is_completed(): branch, which records
Job killed, exit code [...], sets ERROR and attaches process.get_latest_log(max_lines=20).
This has to be inside with TeePopen(...) as process: (lines 469..510) - process is referenced
in _run and nowhere else, so a recovery at :1093 could never reach the log tail.

The RUNNING status is load-bearing, not cosmetic. is_completed() is
status not in (PENDING, RUNNING), so a synthesized ERROR is already completed and skips the
branch above:

synthesized status	is_completed()	enters the log-tail branch?	resulting info
ERROR	True	no	bare Failed to read Result json, ex: [...]
RUNNING	False	yes	Job killed, exit code [...] + the log tail

RUNNING is also what the process actually was: it is precisely the state _pre_run persisted, so
the fallback reconstructs reality rather than inventing it.

3. Make the second read safe (ci/praktika/runner.py) - the same helper at :1093. The
if not res and result.is_ok(): set_status(ERROR) reconciliation right below it is kept exactly
where it is, on the unconditional run path: it is the only conversion of a completed OK result
into ERROR when the run failed (_get_result_object only promotes results that are not
completed), and it must stay outside if run_hooks: so ordinary local runs keep it and it stays
ahead of the on_error_hook.

_get_result_object's own guard is left unchanged - it is the last-resort path, and with no
process in scope there its terminal ERROR is correct. The two workflow-result reads in
_post_run read a different file written by a different process; they are a separate concern and
are not touched here.

4. Keep the hook-less local run red (ci/praktika/runner.py). python3 -m ci.praktika run
calls run with local_run=True and run_hooks=False (__main__.py:396-398), so the
if run_hooks: block - and with it _get_result_object, the only place that promotes a
non-completed result - is skipped. Degrading the read there would report success for a job that
exited 0 but left nothing readable, where before the degradation the unhandled JSONDecodeError
exited 1. So when the hooks are skipped, a synthesized result is promoted to ERROR and clears
res, which restores the old exit code with a readable reason instead of a traceback.

The condition is "the read failed", tracked by an ext marker set only in the fallback - not
not result.is_completed(). A local run whose job writes no result at all legitimately ends on the
RUNNING/PENDING result the pre-run dump persisted and exits 0 on master too, so the broader
condition would turn that pre-existing, unrelated case red. It is also gated on not run_hooks
(the hook path's promotion and exit code are unchanged, measured) and on res (a job that already
failed has been reported by _run, log tail included).

This cannot hide a failure

The job stays red on every path. The fallback's is_ok() is False, so
_get_result_object promotes it to ERROR and _finish_workflow still counts it in
failed_results. A valid result holding a real test FAIL is returned byte-unchanged - there is an
explicit test for that. The change converts a crash with no information into a red job that says
why, and Serializable.from_file still raises: only these two callers decide to degrade.

One intended consequence worth naming: once :1093 no longer aborts, _get_result_object promotes
and dumps the result, so native_jobs.py:1049's not is_completed() stops firing for this
signature and NOT_FINALIZED is no longer recorded. The reason moves from nowhere into the job's own
info.

Testing

Two new pure-Python files under ci/tests/ (which runs on every PR), 31 tests, no build required:

• test_result_dump_atomic.py - a write that dies partway leaves the previous complete content
  readable; the temp file is a sibling of the target and uniquely named per process; no temp is left
  behind on failure; content and mode are identical to the base implementation; a symlink at the
  temp path is refused with ELOOP while both the symlink target and the previously persisted result
  stay intact; a stale regular temp left by a previously killed dump is reused rather than rejected;
  nesting round-trips.
• test_runner_unreadable_result_json.py - empty / truncated-mid-key / truncated-before-brace /
  garbage / missing file all degrade to a RUNNING, is_completed() is False, is_ok() is False
  result with the reason in info; a genuine FAIL and a genuine OK are returned unchanged; AST
  assertions pin that no bare Result.from_fs(job.name) returns to run/_run and that the
  reconciliation stays on the unconditional path, outside if run_hooks:, ahead of
  _get_result_object; two tests drive the real Runner._run in-process over a 0-byte result
  file (no docker, no server, no build) and assert on the result read back from disk - exit 125
  gives ERROR carrying both the read error and the job's own log tail in info plus
  Job killed, exit code [125] in errors, while exit 0 stays RUNNING so promoting it remains
  _get_result_object's job; and one drives the real Runner.run so the second read, the one that
  actually killed the runner, is executed rather than only pinned by AST. The hook-less local path
  has its own group: a job that exits 0 leaving an unreadable result makes the command exit 1
  with ERROR; a readable OK still exits 0; a merely RUNNING result is left alone (the control
  that keeps the condition off is_completed()); the hook path is handed an untouched RUNNING
  result so the promotion stays _get_result_object's; the already-failed path keeps _run's log
  tail; and the marker is present only on a synthesized result and survives the dump round trip.

Both directions: the 31 tests fail 23/31 against unmodified master sources and pass 31/31 with the
change; on master the first _run test fails by raising JSONDecodeError out of runner.py:490
and the run test by raising it out of runner.py:1093, i.e. by the crash itself rather than by an
assertion. 27 mutants were run, each killed by its predicted assertion - including the important one,
synthesizing ERROR instead of RUNNING, which removes the crash while silently suppressing the log
tail, and the closely related one that moves the recovery out of the with TeePopen(...) scope. Both
are caught by "daemon-death-tail" in persisted.info, the first also by is_completed() is False.
Rewriting the second read as the equivalent Result.from_file(Result.file_name_static(job.name)) is a
mutant too: it slips past the AST pin and is caught only by the Runner.run test. Dropping
O_NOFOLLOW, narrowing the mode to 0o600 and using O_EXCL are mutants as well - the first two die
on the symlink test and the mode assertion, the third on the stale-regular-temp test (the symlink test
rejects O_EXCL too, but with EEXIST rather than ELOOP, i.e. for the wrong reason). Six of the
27 target the hook-less local path: deleting the promotion, broadening it to not is_completed(),
dropping the marker, dropping the res guard, dropping the not run_hooks gate, and persisting
ERROR without clearing res. The full ci/tests/ suite was run before and after; the three
pre-existing cases that need a live server on the machine used here flap independently of this diff,
so the clean A/B excludes exactly those: 482 -> 487 passed with 0 failures in both arms, i.e. exactly
the new tests.

Also measured directly on the default local invocation, over six job outcomes (unreadable / garbage /
no-result / failed / OK / failed-with-result): every exit code now matches unmodified master,
with a readable ERROR in place of the traceback in the two unreadable cases.

fsync adds a sync per dump; against an ~80-minute coverage job with a ~1.5 MB result payload
(~32 ms to serialize) that is negligible. It can be dropped if preferred - the atomicity guarantee
comes from os.replace, not from fsync, and no durability beyond that is claimed.

The .gitignore hunk is why running the suite locally is now clean: the pre-existing
ci/tests/test_e2e.py renames ci/tmp to ci/tmp_result (and ci/tmp_backup) in its finally
blocks, and only /ci/tmp was ignored, so both scratch directories showed up as untracked source
after any ci/tests/ run. The test itself is left alone.

Relation to #109673

#109673 labels a docker-daemon-death
truncation as infra so the job is auto-retried. Its label sits inside the
exit_code != 0 / not result.is_completed() branch, which this signature cannot reach today
because the read above it raises. The two are complementary: that PR decides what to do once the
branch is entered, this one makes an unreadable result file reach it at all. Its is_completed()
precondition is provably satisfied by the RUNNING synthesis; no claim is made here about
auto-retry firing end to end.

Prior art for the shape (praktika fix + a ci/tests/ regression test for a docker-daemon-death
class): merged #109096.

No related open issue found for this signature.

[groeneai]

Internal second-model review - adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex; 5 review rounds over the PR's
lifetime; 6 gate findings plus my own cold review each round; 4 fix rounds executed before this
publication). The final round returned zero blockers and zero majors.

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	The tests never execute the runner path that persists the error and attaches the log tail (ci/tests/test_runner_unreadable_result_json.py)	AGREE - fixed @ 03e1f3a1	Two behavioral tests now drive the real Runner._run in-process over a 0-byte result file and assert on the result read back from disk: exit 125 gives ERROR carrying the read error, the job's own log tail and Job killed, exit code [125]; exit 0 stays RUNNING
2	❌	The predictable temp path lets a writable job workspace redirect the staged write through a symlink (ci/praktika/result.py)	AGREE - fixed @ 125b5624	The staged write uses os.open(tmp, O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW, 0o666), so a pre-existing symlink is refused with ELOOP. O_EXCL was rejected as the remedy: it would turn the recoverable leftover-temp state this method exists to survive into a hard FileExistsError
3	⚠️	The second job-result read, the one that actually killed the runner, is protected only by an AST spelling assertion (runner.py:1093 on master)	AGREE - fixed @ 5dd2884e	Result.from_fs(name) is from_file(file_name_static(name)), so the equivalent respelling slips past the AST pin and reinstates the crash. A test now drives the real Runner.run over that read and fails by raising JSONDecodeError if it is unguarded
4	⚠️	Nothing proves the promised recovery from a stale regular temp file (the deliberate absence of O_EXCL)	AGREE - fixed @ 5dd2884e	The pre-existing symlink test rejects O_EXCL too, but with EEXIST rather than ELOOP, i.e. for the wrong reason. A test now pre-creates a partial regular temp at the derived path and asserts the dump lands
5	💡	The new Runner.run test leaks JOB_NAME / CHECK_NAME into later tests in the same worker	AGREE - noted, not blocking	Found independently in my own cold review before the gate ran. Real but latent: the readers are _Environment.from_env (only reached when environment.json is absent, which these tests write), stress_job.py:256, ast_fuzzer_job.py:568 and keeper_stress_job.py:57, and no ci/tests file drives any of those entry points. The one later test that forwards env passes PATH only
6	💡	The new test-module docstrings repeat incident detail from the PR description	DISAGREE	Measured: 17 and 21 lines, not the 42 claimed. They are the norm for this directory (test_ast_fuzzer_memory_limit.py opens the same way) and they carry the two facts not derivable from the code at that spot: that open(..., "w") truncates before the first byte, and that an ERROR fallback is is_completed() == True and therefore skips the log-tail branch
7	💡	os.replace swaps the inode, so the result file's identity, mode and mtime change on every dump	DISAGREE	No consumer depends on any of them: nothing holds an fd across a dump, no file-granularity bind mount of the result JSON exists (the two --volume result mounts are directories), nothing tails or inotifys it, nothing reads st_ino/st_mtime, and StorageUsage reads only getsize. The from-root chown covers the whole temp directory rather than an inode, and every job recreates ./ci/tmp
8	💡	The temp name is 12 bytes longer than the target, so a near-NAME_MAX result name would newly raise ENAMETOOLONG	DISAGREE	NAME_MAX is 255 and the longest job-derived result filename in ci/defs + ci/workflows is 58 bytes, a ~185-byte margin. The names that do get long are sub-result names derived from error messages, and those never own a file, so dump() is unreachable for them
9	💡	The RUNNING fallback fabricates start_time, so the job reports a duration of seconds instead of ~5800s	DISAGREE	Real and accepted. The true start_time lives only in the file that is by definition unreadable on this path, and the environment carries no job start time, so there is no cheap recovery; leaving it None is worse, since the duration update and the CIDB insert both dereference it. A wrong duration on a job that is now correctly red and explains itself beats a blank red box
10	💡	.gitignore's two new entries were unexplained in the description	AGREE - fixed in the description	The pre-existing ci/tests/test_e2e.py renames ci/tmp to ci/tmp_result and ci/tmp_backup in its finally blocks and only /ci/tmp was ignored; the test itself is untouched

Severity: ❌ blocker / ⚠️ major / 💡 nit. DISAGREE verdicts carry recorded evidence and are terminal
per finding. Nine further nits raised across the five rounds were description-accuracy corrections
(test counts, mutant counts, the deliberately out-of-scope sibling serializers) and were fixed in the
description rather than in code.

Session id: cron:clickhouse-review-slot-50:20260728-085200

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, 100%, no build needed. From the repo root: create the 0-byte result file the daemon death leaves (open(Result.file_name_static(name), "w").close()) and call Result.from_fs(name) - it raises JSONDecodeError: Expecting value: line 1 column 1 (char 0), byte-identical to the job log. Also reproduced for a truncated JSON, and for the destruction step itself (dump a valid RUNNING result, raise inside json.dump: the file drops from 260 to 12 bytes and the previous result is gone).
b	Root cause explained?	open(..., "w") in Serializable.dump truncates the result file to 0 bytes before writing -> a docker-daemon death mid-dump leaves it unreadable and destroys the RUNNING result _pre_run persisted -> Result.from_fs raises -> the second job-result read in Runner.run (runner.py:1093) is unguarded, while its sibling at :548 is guarded, so the exception escapes and kills the runner before any reporting. Verified by AST: :490's raise is caught by the try whose body is 1071..1086; :1093 sits outside it. _finish_workflow then stamps ERROR/NOT_FINALIZED with no info.
c	Fix matches root cause?	Yes: the destructive write window is removed (atomic Result.dump) and an unreadable file is made non-fatal and reportable at the one place where the log tail is reachable. Not a band-aid: Serializable.from_file still raises, so only these two callers decide how to degrade; no bound widened, no exception swallowed.
d	Test intent preserved / new tests added?	No existing test weakened or removed - the full ci/tests/ failing set is identical before and after, and with the three host-state-dependent files ignored the passed count moves 477 -> 479 (0 failed in both arms), i.e. exactly the new tests. 23 new tests in two files: the controls that a genuine FAIL and a genuine OK result are returned byte-unchanged, two end-to-end tests that drive Runner._run itself and assert on the persisted result, one that drives the real Runner.run over the second job-result read (the read outside all three of its try blocks, which the AST pin alone cannot cover because Result.from_file(Result.file_name_static(job.name)) unparses differently), a symlinked-temp refusal test, and one asserting a stale regular temp left by a killed dump is reused rather than refused. One existing test was strengthened, not weakened: the base-implementation parity test now also compares the file mode.
e	Both directions demonstrated?	Yes: 18 of the 23 fail against unmodified master sources, 23/23 pass with the change; reproduced twice (source patch reverted in-tree, and independently against a git archive origin/master ci export). The two _run tests fail on master by letting JSONDecodeError escape _run at runner.py:490 - the crash itself, not an assertion; the symlink test fails with DID NOT RAISE <class 'OSError'> while the victim file is clobbered. Mutation matrix (21 mutants), each killed by a named assertion: returning ERROR instead of RUNNING from the helper, and moving the recovery out of the with TeePopen(...) scope (losing process.get_latest_log()), both die on "daemon-death-tail" in persisted.info; deleting set_status(ERROR) dies on status == ERROR; dropping O_NOFOLLOW dies on the symlink refusal test while adding O_EXCL dies with FileExistsError (errno 17) on the stale-regular-temp test, so the two are independently load-bearing; rewriting the second job-result read as Result.from_file(Result.file_name_static(job.name)) keeps the AST pin green and dies on the Runner.run test by raising JSONDecodeError; a 0o600 temp mode dies on assert '0o600' == '0o664' in the base-parity test. O_EXCL turning the recoverable state this method exists to survive into a hard failure is now pinned by a test rather than only measured.
f	Fix is general across code paths?	All three job-result reads enumerated by AST: :490 recovered, :1093 recovered, :548 already guarded and deliberately left alone (no process in scope there, so its terminal ERROR is correct). An AST test fails if a bare Result.from_fs(job.name) reappears in run/_run, and a behavioral test fails if the recovery leaves the TeePopen scope. On the write side Result has no other dump(), so one override covers all ~20 Result write sites, and the single open() this diff introduces is the only new write surface (the target itself needs no symlink guard: os.replace does not follow a symlink at the destination). The workflow-result reads in _post_run are a different file written by a different process - a separate concern, not widened into this PR.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	The unreadable class is parametrized over empty / truncated-mid-key / truncated-before-brace / garbage / missing file. The write side is covered for an empty result, a result carrying 50 sub-results (the real failure carried 11882), and byte-equality against the base implementation's output. Root-only permission cases are deliberately excluded: ci/tests runs as root, where such a case is inert.
h	Backward compatible? (maintainer-approved exception only)	Yes. No setting, no serialization-format change, no SettingsChangesHistory.cpp entry, no user-visible behaviour. The on-disk content is byte-identical to the previous implementation (asserted); only the write is staged.
i	Invariants and contracts preserved?	dump() keeps its contract (returns self, target holds the full serialization) and strengthens it: the target is never partial. _read_job_result_or_running always returns a usable Result, never None, because run calls .is_ok() on it one line later - asserted. The completed-OK -> ERROR reconciliation stays on the unconditional path and ahead of _get_result_object, hence ahead of the on_error_hook; both pinned by AST tests. Error paths: the temp file is removed on any exception (except BaseException) and the exception is re-raised, never swallowed; info is set through the constructor rather than a setter, because the setters would try to dump to the very file that is unwritable. Concurrency: the temp name carries the pid, and no praktika thread body calls Result.dump().

Session id: cron:clickhouse-review-slot-50:20260728-085200

[groeneai]

cc @maxknv - could you review this? When the docker daemon dies mid-Result.dump() the job result JSON is left at 0 bytes, and the unguarded second Result.from_fs in Runner.run then kills the runner before any reporting runs, which is why these coverage-shard jobs go red with an empty info. This makes the dump atomic and degrades an unreadable result to RUNNING so the existing branch can attach the log tail.

[clickhouse-gh]

Workflow [PR], commit [e2910ba]

Summary: ✅

---

AI Review

Summary

This PR makes Result.dump atomic and routes unreadable job-result reads through a recovery path so Praktika can still finalize and report the job instead of dying on JSONDecodeError. On the current head, I did not find any new blockers or majors: the earlier bot finding about the hook-less local path is fixed, and the current runner.py / result.py logic is coherent with the added coverage.

Missing context / blind spots

• ⚠️ I could not run the added pytest files directly because this environment does not have the pytest module installed. I did run a focused manual smoke check covering the atomic overwrite path, the symlinked-temp refusal, unreadable-result degradation to RUNNING, and _get_result_object finalization on the unreadable-success path.

Final Verdict

✅ No new blockers or majors.