fix(scheduler): write run history as one complete JSONL line - #99
fix(scheduler): write run history as one complete JSONL line#99SebTardif wants to merge 1 commit into
Conversation
appendHistory encoded a RunRecord directly onto an O_APPEND file and discarded Close errors. A short write or failed flush could leave a truncated last line. ReadHistory fails closed, so crawlctl run, status, and logs refuse to start. Encode to a buffer, write one complete JSONL line, and return Close errors. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed August 30, 2026, 11:15 PM ET / August 31, 2026, 03:15 UTC. ClawSweeper reviewWhat this changesThe branch buffers scheduler run records before appending JSONL history, returns close errors, and adds normal-write and close-error tests. Merge readiness⛔ Blocked until stronger real behavior proof is added - 5 items remain The PR improves close-error reporting but does not prevent a partially accepted history write from leaving an unreadable trailing JSON record. Because the reader remains fail-closed, the stated scheduler-bricking failure remains possible. Priority: P1 Review scores
Verification
How this fits togetherThe scheduler executes configured crawl jobs and records results in a local JSONL history file. The run, status, and logs commands read that file, so an invalid trailing record blocks those workflows. flowchart LR
A[Configured crawl job] --> B[Scheduler executes job]
B --> C[History append]
C --> D[Local JSONL history]
D --> E[History reader]
E --> F[Run status and logs]
C --> G[Write failure]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Use a crash-safe history update that preserves the last valid history state, such as transactional same-directory replacement, while retaining the existing fail-closed policy for genuinely malformed persisted history. Do we have a high-confidence way to reproduce the issue? Yes, from source: a writer that accepts a byte prefix and returns an error leaves that prefix in history, after which the reader rejects it. The supplied runtime trace covers only a normal successful append. Is this the best way to solve the issue? No. Buffering alone does not recover a partial write; the persistence strategy must preserve a valid prior state or safely recover the final incomplete record. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 52f0fb12e4d2. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
clawsweeper
Bot
added
merge-risk: 🚨 availability
|
Maintainer triage: CLOSE recommended for the stated partial-history-write fix at I reproduced the failure with real Both versions produced the same result: Normal runs and history reads passed on both versions. A separate malformed-history fixture also correctly blocked a marker-writing job. All test data was temporary; parent process limits were unchanged. The root cause is a partial append remaining in a file whose reader rejects any malformed record. Go's JSON encoder already buffers the complete record and newline before one Write, so the added buffer provides no additional partial-write protection. This is an unresolved existing failure, not a new corruption regression introduced by the contributor. Returning close errors is a useful narrower improvement, but does not establish the reliability claim. Independent Codex autoreview, covering priorities P0–P3, reached the same conclusion and identified no other distinct actionable defect. The existing Linux and Windows CI run is green, but its tests do not establish recovery from a real partially accepted write. I recommend a separately scoped decision about crash-safe persistence versus recovery of an incomplete trailing record before replacing the history strategy. No contributor changes are requested by this triage. Minimal real-binary reproducerBuild each revision with from pathlib import Path
import resource
import signal
import subprocess
import sys
import tempfile
binary = str(Path(sys.argv[1]).resolve())
for allowance in (80, 120):
with tempfile.TemporaryDirectory(prefix="crawlkit-history-proof-") as temp:
root = Path(temp)
env = {"PATH": "/usr/bin:/bin", "LANG": "C"}
for key in ("HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME"):
directory = root / key.lower()
directory.mkdir()
env[key] = str(directory)
cfg = root / "crawlctl.toml"
cfg.write_text('version = 1\n[jobs.ok]\nenabled = true\n'
'command = ["/usr/bin/true"]\n')
argv = [binary, "--config", str(cfg)]
def run(*args, limited=False):
def constrain():
signal.signal(signal.SIGXFSZ, signal.SIG_IGN)
resource.setrlimit(resource.RLIMIT_FSIZE, (limit, limit))
return subprocess.run(
argv + list(args), cwd=root, env=env,
capture_output=True, text=True, timeout=20,
preexec_fn=constrain if limited else None)
seed = run("run", "ok")
assert seed.returncode == 0, seed.stderr
history = root / "state" / "runs.jsonl"
original = history.read_bytes()
limit = allowance if sys.platform == "darwin" else len(original) + allowance
failed = run("run", "ok", limited=True)
data = history.read_bytes()
print("allowance", allowance, "exit", failed.returncode, failed.stderr.strip())
print("prior_preserved", data.startswith(original),
"complete_records", len(data.split(b"\n")) - 1,
"trailing_bytes", len(data.rsplit(b"\n", 1)[-1]),
"newline", data.endswith(b"\n"))
for args in (("status",), ("logs",), ("run", "ok")):
result = run(*args)
print(" ".join(args), "exit", result.returncode, result.stderr.strip()) |
|
Thanks, @SebTardif, for investigating the scheduler history failure and adding coverage. We're closing this PR because the added buffering duplicates what |
2 participants
What Problem This Solves
Fixes an issue where operators who run
crawlctl run(directly or from launchd/systemd) can get a bricked scheduler after a short write, crash, or failed close while appendingruns.jsonl.ReadHistoryfails closed on any unmarshal error, socrawlctl run,status, andlogsthen refuse to start until the file is hand-edited.Why This Change Was Made
appendHistorynow encodes eachRunRecordinto a buffer, writes that one complete JSONL line, and returnsCloseerrors instead of encoding directly onto theO_APPENDhandle and discarding flush failures. The on-disk format and the fail-closed read contract are unchanged. Existing already-truncated files still need a manual edit.User Impact
A successful
crawlctl runleaves a complete last line instate/runs.jsonl, so a laterrun,status, orlogscommand can read history again. Operators still seeerror: unexpected end of JSON inputif the file is already truncated from an older build.Evidence
Public
crawlctlagainst a temp config on the patched binary. A truncated last line still bricksrunandstatus(fail-closed). After a successful job, history is one newline-terminated JSON object andstatusreads it.Real behavior proof
Behavior or issue addressed:
crawlctl runcan leave a truncated lastruns.jsonlline; laterrun,status, andlogsthen fail closed until the file is edited.Real environment tested: macOS darwin/arm64, Go 1.27.0, crawlkit worktree
/tmp/oc-pr-crawlkit-F001at branchfix/f001-atomic-run-history, publiccrawlctlbuilt withGOWORK=off go build -o /tmp/crawlkit-F001-live/crawlctl ./cmd/crawlctl.Exact steps or command run after this patch: Built
crawlctl, pointed--configat a tempcrawlctl.tomlwithjobs.ok.command = ["true"], wrote a truncatedstate/runs.jsonl, rancrawlctl runandcrawlctl status, removed the bad file, rancrawlctl runagain, then inspected the written line withpython3.Evidence after fix: terminal output from the patched
crawlctl:Observed result after fix: After a successful job,
runs.jsonlis one complete JSONL line andcrawlctl statusprintsok: success. A truncated last line still returnserror: unexpected end of JSON inputand does not start jobs.What was not tested: A hard crash mid-
Writeon NFS, launchd/systemd scheduling, and Windows close-flush behavior.Summary
The hole has been present since
d41253c(feat: add crawlctl scheduler, 2026-05-22, 99 days).ReadHistoryfail-closed behavior is unchanged and is still covered byTestRunReturnsHistoryReadErrorBeforeRunningJobs.