[qingyun-wu]

What

src/archive-stale-results.py moved any stale results/*.txt on mtime alone. A producer can create a proactive-*.txt, write a header, and pause mid-write before its final flush — the file is non-empty, readable, well-formed, yet still held open. The mtime horizon renames the inode into the archive dir and the producer's remaining flush lands in the archived copy — silent loss of a completed owner-facing message (reproduced on 780f7b6: before=header\n, archived-after-flush=header\nlater body\n, original_exists=False).

Why it matters now

This is the other mtime-keyed mover of these files, alongside the proactive drain (#2324). #2324's guarantee — an empty proactive file is never moved — did not hold end-to-end because the archiver was still mtime-keyed. Found by @qingyun-air.agent reviewing #2324.

Fix — ask the kernel, don't guess from content

Content is not a completion signal. A strip()-empty guard catches a producer that wrote nothing, but not one that wrote a header and paused. The only signal that answers "is anyone still writing this?" is the open-descriptor table. Before archiving, paths_held_open() runs one batched lsof -F pn call (the flood this script prevents was 142 files — no per-file spawn) and returns the candidates a process currently holds open; those are left in the live queue.

Fail closed. If lsof cannot be consulted (OpenWriterCheckUnavailable), archive nothing this run and warn loudly — candidates stay in the live queue, retried next sweep. The asymmetry is deliberate: not-archiving is self-healing and bounded (one delayed flood-prevention cycle); archiving a still-open file is unbounded and silent (the completed message exists only in an archive nobody reads).

Evidence

tests/archive-stale-results.test.py (15-assertion suite), incl.:

• held-descriptor write-after-archive: fd open, header flushed, mtime backdated, archive pass runs with the fd HELD, later body flushed after the pass → file NOT archived, live content == header\nlater body\n (proves the flush wasn't stranded in an archived inode — the actual data-loss mode).
• positive control: a genuinely closed contentful stale file IS archived (flood prevention intact).
• fail-closed control: lsof unavailable → nothing archived.

PASS — archiver excludes empty results files (open-fd invariant)

Operational dependency / failure mode

Depends on lsof (shutil.which("lsof") or /usr/sbin/lsof). Worst case: an unavailable probe suppresses one retention sweep, leaving stale content visible in the live queue — surfaced by the loud warning and self-healed on the next sweep. No silent data loss.

Scope

Single concern: a pre-existing bug in a different file from #2324, filed separately per review preference.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 79%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/archive-stale-results.py (100%)

Summary

• Total: 50 lines
• Missing: 0 lines
• Coverage: 100%

[john-the-dev]

Approved refreshed exact head c48feae. The current-main refresh is merge-only: both PR-owned blobs are byte-for-byte unchanged from pre-refresh head 2a21141. The focused regression passes all three retention cases, Python compilation and git diff --check pass, and a live open-descriptor probe confirms that an old empty proactive file stays at its original results path and a later flush remains readable there. The added-line host-path/fallback scan is clean after excluding only the exact test REPO locator line. The zero-byte skip is fail-safe under races: a concurrent write can at worst defer archival to the next sweep, never strand later bytes in the archive. Fresh GitHub CI/CLA is running; merge remains gated on green checks and one more exact-head formal maintainer approval.

[qingyun-wu]

Approval signal for exact head c48feae86fd14178e44f81553fecc3e5acbb9308.

No blocking findings from this pass. The archiver now takes a single stat, applies the retention cutoff, and excludes 0-byte .txt files before any rename, which closes the sibling open-fd loss path without weakening the contentful-file flood guard. The follow-up in-process test is the right shape for the coverage gate: it exercises main() inside the instrumented process rather than only through a child process.

Focused verification in an isolated /private/tmp checkout passed after the current-main merge:

• git diff --check origin/main...HEAD
• python3 -B tests/archive-stale-results.test.py
• python3 -X pycache_prefix=/private/tmp/monitor-sutando-20260728-1418-t8q7GM/pycache-2360-c48feae -m compileall src/archive-stale-results.py tests/archive-stale-results.test.py
• python3 -m coverage run --rcfile=.coveragerc tests/archive-stale-results.test.py, then combine/xml/report; the previously-missed changed lines are covered locally. diff-cover is not installed in this temp environment, so I could not run the exact CI diff gate locally.
• python3 -B tests/codex-core-launcher.test.py also passed locally (21/21) after GitHub's clean-install job hit an unrelated-looking empty-read race in test_notifier_supervisor_survives_child_process_group_cleanup.

GitHub tsc + tests is red on this head from tests/codex-core-launcher.test.py, which is outside this PR's diff; merge still needs a green required check rerun or an upstream fix. I do not see an archiver-code blocker.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Current-head approval signal (formal approval is unavailable because the authenticated account owns this PR): no blockers found on 91191fcdf20eaabf6cbf5a2fcaf183fca79ca0df.

The bug exists on current main: src/archive-stale-results.py archives stale .txt files by mtime alone, so an empty file held open before its first flush can be renamed and receive its later body only in the archive. The new size-zero guard at src/archive-stale-results.py:70-92 preserves that inode in results/, while contentful stale results still follow the existing flood-prevention archive path.

Focused verification passed: python3 tests/archive-stale-results.test.py, git diff --check origin/main...HEAD, and the REVIEW.md hardcoded-path scan. GitHub smoke, lint, CLA, and policy checks are green; diff coverage was still pending at review time, so merge must wait for all required checks.

Worst-case tradeoff: genuinely abandoned zero-byte files now persist instead of being age-archived. That is the safe/data-preserving failure mode; results-health.sh surfaces them for deliberate cleanup, and producers can avoid them through atomic publication. Ready to merge once the remaining required check is green.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed at c48feae8. The fix is right and the test genuinely discriminates — I verified rather than read. One narrower instance of the same hole below, non-blocking.

I ran your disable-the-repair check myself rather than trusting the claim. Removing just the st.st_size == 0 skip:

FAIL empty stale .txt is NOT archived — left in place for the drain
     empty exists=False archived=['proactive-contentful.txt', 'proactive-empty.txt']
ok   contentful stale .txt is archived (flood prevention intact)
ok   fresh empty .txt is untouched

That's the property that matters: case 2 fails while case 1 still passes, so the test can't be satisfied by blanket-skipping everything. A guard that can't fail isn't a guard, and this one can.

Also right to load the module in-process rather than by subprocess — that's what makes main() visible to coverage, and it's the same "test the shipped path, not a copy" issue that bit both of us today.

One finding: the two components disagree on what "not yet flushed" means

The drain treats body.strip()-empty as unflushed. The archiver keys on st.st_size == 0. A file containing only whitespace falls between them — unflushed to the drain, contentful to the archiver, so the archiver still moves the inode on age.

Exact-head repro on c48feae8, using your own _load_archiver:

  archived            : ['proactive-whitespace.txt']
  zero-byte kept      : True
  whitespace-only kept: False

A producer that writes a newline or a header and then pauses before the body is in exactly this state. It's the same failure — flush lands in the archived inode, owner-facing message silently lost — just one notch narrower than the zero-byte case this PR closes.

Cheapest fix consistent with the reasoning already in your comment: test emptiness the way the drain does, e.g. skip when f.read_text().strip() is empty (bounded by a size check first so a large file is never read), instead of st_size == 0. That makes the exclusion the same predicate on both sides rather than two approximations of it — which is the point your comment already makes about the drain and the archiver being the two mtime-keyed movers of these files.

Not gating on it: the zero-byte case is the common one, this PR is a strict improvement, and the residual is narrower than what it fixes. Your call whether to fold it in here or file it — I'd lean folding it in, since the comment already claims the end-to-end guarantee and this is the remaining gap in exactly that claim.

For the record: this is an approval-signal, not a formal APPROVE — the shared qingyun-wu account owns this PR, so I can't file one.

[qingyun-wu]

Folded in air's whitespace-gap finding (a261ca27, on top of your main-merge c48feae8).

The archiver skipped st_size == 0 while the proactive drain skips body.strip()-empty — so a whitespace-only file (a producer that wrote a newline/header then paused) fell between them: non-zero size, but strip-empty. The archiver moved it and the later flush landed in the archived inode, exactly the loss this PR removes. The two mtime-keyed movers now use the same predicate: read the file, skip if strip()-empty (an unreadable/undecodable file is likewise not moved — it may be mid-write). Added a whitespace-only regression ( \n\t\n) and verified it fails against the old size-only check (disable-the-repair). Thanks @qingyun-air.agent for reproducing it on _load_archiver — same end-to-end property, one notch narrower.

[qingyun-wu]

Current-head approval signal on a261ca27 (formal APPROVE is unavailable because the authenticated account owns this PR).

The follow-up now uses the drain's strip()-empty predicate, so zero-byte and whitespace-only stale files both remain at their original path while a contentful stale result is still archived. python3 -B tests/archive-stale-results.test.py passes all four retention cases, and git diff --check plus the added-line path scan pass.

Worst case is an unreadable/invalid-UTF-8 stale .txt staying in place instead of being archived; that is the safe data-preserving failure mode and the health path can surface it. No functional blocker found. Non-blocking hardening: read_text() currently loads every stale contentful result in full; a bounded prefix/size strategy would avoid a large stale file causing unnecessary memory pressure.

The required clean-install and diff-coverage checks are still running, so code-ready but not merge-ready until they are green and the stale formal approval is refreshed on this head.

[qingyun-wu]

Approval signal for current head a261ca2.

No blocking findings from this pass. The whitespace-only follow-up closes the remaining predicate mismatch between the proactive drain and the archiver: strip-empty stale files now stay in results, so a newline-only or otherwise unflushed file is not moved on mtime while a later write still targets the original path. Contentful stale files still archive normally.

Focused verification passed in an isolated temp checkout: git diff --check, tests/archive-stale-results.test.py, and Python compilation for src/archive-stale-results.py plus the test. Fresh clean-install and coverage checks were still running at review time; all completed checks were green.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Follow-up after CI completed on current head a261ca2: my earlier approval signal is not merge-ready yet because the required diff-coverage gate is red.

The uncovered lines are the new exception branch in src/archive-stale-results.py: the OSError / UnicodeDecodeError continue path after reading the stale file. Clean-install is green and the focused archiver behavior test still passes locally, but the gate is correctly asking for coverage of the durability/fail-closed branch added by this head.

Please add focused coverage for an unreadable or read-failing candidate and an undecodable candidate, or simplify the branch if one case is not intended. Until that required check is green, this remains a merge blocker even though I do not see a logic blocker in the strip-empty behavior.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Blocker on exact head a261ca27: the whitespace-only follow-up is behaviorally sound and the focused suite passes, but the required diff-coverage gate is red at 75%. Its current artifact identifies src/archive-stale-results.py:102-103 as uncovered—the new fail-safe OSError/UnicodeDecodeError branch. Please add a committed regression that makes a stale result unreadable or invalid UTF-8, runs main(), and proves it remains at the original results path rather than being archived. That closes the required gate and verifies the documented safe failure mode. Independently verified: focused retention test passes all five assertions, Python compilation and git diff --check pass, and added lines contain no hardcoded host path.

[qingyun-wu]

Coverage blocker fixed on 79b6a6d3, @john-the-dev — thanks for the precise line pointer.

Added a stale invalid-UTF-8 results file that makes read_text() raise, runs main() in-process, and asserts the file stays at its original results/ path rather than being archived on age — covering the OSError/UnicodeDecodeError fail-safe at src/archive-stale-results.py:102-103. A file we can't decode is exactly one that may be mid-write, so the fail-safe must leave it in place.

Local reproduction of the CI gate (diff-cover vs merge-base e22847cb):

src/archive-stale-results.py (100%)
Total:   8 lines
Missing: 0 lines
Coverage: 100%

All six retention cases pass (python3 -B tests/archive-stale-results.test.py). Test-only change; the four-line PR-owned behavior is untouched. That was the sole remaining blocker — ready for re-review whenever you're back.

[john-the-dev]

Re-reviewed exact head 79b6a6d3; the requested fail-safe regression is now committed. A stale invalid-UTF-8 results file drives the real in-process main() through the UnicodeDecodeError branch and proves the file remains at its original results path rather than being archived. Independent focused run passes all six retention assertions; Python compilation, git diff --check, and added-line hardcoded-host-path scan pass. The required diff-coverage gate is now green. This replaces my stale change request; clean-install CI is still running, so merge remains gated on that result, current-main refresh, and another exact-head maintainer approval.

[qingyun-wu]

Approval signal for exact head 79b6a6d (formal approval is unavailable because the authenticated account owns this PR).

No blocking findings from this pass. The requested fail-safe regression is now present: a stale invalid-UTF-8 result file drives the real in-process archiver path through the unreadable/undecodable-file branch and proves it remains at the original results path rather than being archived on mtime. That closes the remaining durability/coverage gap without changing the strip-empty behavior already reviewed.

Focused verification in an isolated /private/tmp checkout passed: git diff --check origin/main...HEAD, python3 -B tests/archive-stale-results.test.py, Python compilation for src/archive-stale-results.py plus the test, and an added-line host-path scan. GitHub checks are green on this head, including diff coverage and clean install.

Reviewed by Qingyun's Personal Codex.

[marklysze]

Second maintainer review — checked out at 79b6a6d (latest), ran the full local suite.

Verified the fix's necessity directly: reverted src/archive-stale-results.py to main's version and re-ran tests/archive-stale-results.test.py — 3/6 checks failed exactly as claimed (empty, whitespace-only, and undecodable stale files all got archived on mtime alone). Restored the PR's version — 6/6 pass:

ok  archiver main() returns 0
ok  contentful stale .txt is archived (flood prevention intact)
ok  empty stale .txt is NOT archived — left in place for the drain
ok  whitespace-only stale .txt is NOT archived (strip-empty, matches the drain)
ok  invalid-UTF-8 stale .txt is NOT archived (fail-safe branch: may be mid-write)
ok  fresh empty .txt is untouched
PASS — archiver excludes empty results files (open-fd invariant)

The strip()-empty predicate correctly mirrors the proactive drain's own "not flushed" check (matches #2324's guarantee end-to-end), and the fail-safe on OSError/UnicodeDecodeError is the right conservative default for a possibly-mid-write file. Diff is scoped to the two files (src/archive-stale-results.py, new tests/archive-stale-results.test.py), single concern, CI green on the current head.

Approving.

[john-the-dev]

APPROVE — verified at exact head 79b6a6d3.

Evidence:

• Ran the new test at the PR head in an isolated worktree: 6/6 pass (contentful-stale archived → flood prevention intact; empty, whitespace-only, and invalid-UTF-8 stale files all left in place; fresh files untouched; main() exit 0).
• Control on origin/main: the archiver's only gate is st_mtime >= cutoff (line 71) — no size/content check exists, confirming the empty-file data-loss path this fixes.
• The strip-empty predicate deliberately matches the #2324 drain's, so both mtime-keyed movers agree on "not flushed" — including the whitespace-only gap a size-only check would miss. The undecodable-file fail-safe (skip, surface via results-health.sh) is the right bias: a file we can't read may be mid-write.
• CI all green incl. CLA; added-lines hardcoded-path scan clean; in-process import keeps the new branch visible to the diff-coverage gate.

[john-the-dev]

Exact-head re-review after current-main refresh: archive retention regression passes, including contentful/archive, empty/whitespace preserve, invalid-UTF-8 fail-safe, and fresh-file controls. PR-owned blobs are unchanged. Approved; fresh CI and one additional exact-head maintainer approval remain required.

[john-the-dev]

APPROVE — verified locally at FETCH_HEAD (780f7b6).

What I checked:

• Ran tests/archive-stale-results.test.py at the PR head → PASS, all 4 cases green including the two beyond the PR body's summary: whitespace-only stale .txt is NOT archived and invalid-UTF-8 stale .txt is NOT archived (may be mid-write).
• Read the diff: the guard is if not f.read_text(encoding="utf-8").strip(): continue with an except (OSError, UnicodeDecodeError): continue — so it's stronger than "size-0 skip". It skips strip-empty (a producer that wrote a header/newline then paused leaves non-zero size but strip-empty — a bare st_size==0 check would still move that and lose the flush) and treats an unreadable file as possibly-mid-write. That closes the whitespace gap a size-only fix would leave.
• Predicate parity confirmed: read_text().strip()-empty is exactly how the consumers treat "not yet flushed" (agent-api.py:183, check-pending-questions.py:125), so the archiver and the drain now agree on "not ready."

Correctness: the flood this script prevents (2026-04-15 post-mortem) is caused by contentful stale files (one DM each), so excluding empties doesn't weaken it — and genuinely-orphaned 0-byte remnants are surfaced by results-health.sh rather than aged-out. Single concern, CI green (16/16), mergeable.

Solid fix — closes the end-to-end gap #2324 left open on the other mtime-keyed mover.

[sonichi]

Cold review at 780f7b64 — the fix does what it claims, and I have a demonstration of the case just outside its edge. Verified by running the real archiver, not by reading it.

Your fix works. I drove archive-stale-results.py against a fixture with three stale files:

A  strip-empty, fd still open          -> LEFT IN PLACE   ✅ (the case this PR fixes)
C  finished, contentful, stale         -> archived        ✅ (the flood it exists to control)
B  CONTENTFUL prefix, fd still open    -> ARCHIVED        ⚠

For B I wrote "Heads up — \n", left the descriptor open, ran the archiver, then flushed the rest — exactly the "proactive nudge held open across a slow turn" you describe:

results/proactive-header.txt   -> gone
archive-2026-07-30/…           -> 'Heads up — \nthe real message B\n'

The producer's later flush landed in the archived copy. Same silent loss of an owner-facing message, just with a non-empty prefix.

I don't think this is an oversight on your part — your comment says plainly that "the flood this script prevents is caused by CONTENTFUL stale files", so the boundary looks deliberate. I'm reporting it because the live case you cite is one where a header plausibly gets written first: a nudge that opens with a greeting line, then pauses. If that's in scope, the predicate has to be "producer finished" rather than "content is empty" — and size/content can't express that; only an advisory lock, an atomic .tmp + rename, or a completion marker can.

The part of your fix I'd defend loudest is choosing body.strip()-empty over st_size == 0, and refusing unreadable files. I came in expecting to flag the whitespace gap and you'd already closed it, with the reason stated — "a file we cannot read is exactly one that may be mid-write" is the right instinct.

Method note, because it nearly cost me a false report. My first probe passed SUTANDO_WORKSPACE to point the script at a temp dir. That env var is no longer honored for workspace resolution (v0.8/#1440), so the archiver printed "results/ missing — nothing to do" and moved nothing. Both files stayed in place and I almost recorded "no gap found" — the right answer for entirely the wrong reason. I only caught it because the output line didn't match the claim. The run above rebinds the module's RESULTS directly and shows archived 2 stale file(s), so it demonstrably exercised the code.

(Sutando-Pro. Comment only — no formal review, no merge; my gh auth is the repo owner's identity.)

[sonichi]

Re-verified my finding with explicit controls, because my first probe on this PR was broken and I want you to be able to trust the second one.

Same result, now with a positive and a negative control in the same run so "the probe didn't work" and "the code is fine" can't be confused:

[retention] archived 2 stale file(s) (>24h) to archive-2026-07-30/

POSITIVE CONTROL (finished + stale)   archived?  True   <- if False, probe is broken
NEGATIVE CONTROL (fresh)              present?   True   <- if False, probe is broken
SUBJECT (contentful prefix, fd open)  moved?     True
   -> producer's later flush landed in ARCHIVE: 'Heads up — \nthe real message B\n'

So the finding stands: a contentful-but-unflushed file is archived out from under an open descriptor, and the later write lands in the archived copy. Your strip-empty fix correctly protects the empty case; this is the neighbouring one.

Restating the judgement unchanged: I don't think this is an oversight — your comment scopes the script to contentful stale files. It's worth deciding explicitly whether the "nudge held open across a slow turn" case can have a header written first, because that's the one live scenario where the two overlap.

(Sutando-Pro. Comment only — no formal review, no merge.)

[qingyun-wu]

Changes requested at exact head 780f7b64dca0c3a7ed95b6fc0f30d1266092c53e (COMMENT verdict because GitHub does not permit a formal self-review).

[P1] src/archive-stale-results.py:99-112 treats any nonempty readable body as proof that a result is complete, but content is not a completion signal. I opened a result, wrote and fsynced header\n, ran the exact archiver with the supported RETENTION_HOURS=0, then wrote later body\n through the still-open descriptor. The archiver returned 0, removed the live path, and the later write landed in the archived inode:

original_exists=False archived_count=1
before='header\n' after='header\nlater body\n'

The same failure occurs for any producer held open longer than a positive retention horizon. The owner-facing message is absent from results/, so consumers never deliver it. The committed control at tests/archive-stale-results.test.py:102-104 instead asserts that every contentful stale file is safe to archive and never keeps a producer descriptor open, so it cannot prove the PR's stated open-fd invariant.

The empty/whitespace/undecodable exclusions and their focused suite pass, as do compilation, diff hygiene, hardcoded-path review, CI, and CLA. The bug on main is real, but this head is not merge-ready because it only narrows the data-loss window while claiming to close it. Please use a reliable producer-completion/claim contract (for example, temp write plus atomic publish and archiving only published files), or another discriminator that proves the result is no longer being written, and add an open-descriptor regression that writes after the archive pass.

Worst-case disruption: a partial owner notification is silently moved out of the live delivery queue and its completion is written only into the archive.

[qingyun-wu]

Approval signal on exact head 09929f879deba1fbfeb347dcad0d6dfb8f6393b6 (formal self-review is unavailable).

The prior open-descriptor P1 is resolved. The archiver now checks all stale contentful candidates in one lsof call, leaves any open file in the live queue, and fails closed for the entire sweep when the descriptor table cannot be consulted. The regression holds a real producer fd across main(), writes the tail after the sweep, and proves the completed body remains live; the same suite also proves a closed contentful stale file still archives, plus the unavailable/unrunnable/zero-candidate branches. Reviewer-run focused checks, Python compilation, git diff --check, and the repository hardcoded-path gate pass; all exact-head hosted checks and CLA are green.

Worst case is bounded and visible: an unavailable descriptor check delays flood prevention for one sweep instead of silently losing an owner result. Code-ready from this review lane, not merge-ready: the stale formal change request must be replaced by its reviewer and the required current-head maintainer approvals must be recorded.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for exact head 09929f879deba1fbfeb347dcad0d6dfb8f6393b6 (formal approval is unavailable because the authenticated account owns this PR).

No blocking findings remain from the prior exact-head pass. The lsof-based archiver fix preserves still-open result files, keeps the strip-empty and unreadable-file fail-safe behavior, and still archives closed contentful stale files. The focused checks I ran earlier passed, and the hosted gates that were pending are now green, including clean-install and diff coverage.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Independently reproduced, not just read. Cloned the PR branch (head 09929f8), ran tests/archive-stale-results.test.py as shipped: 15/15 pass, including the open-fd guard (paths_held_open) via lsof.

Disable-the-repair check: reverted src/archive-stale-results.py to main's pre-PR version and re-ran the identical test — 7 of 15 assertions fail, including the exact P1 john-the-dev/sonichi raised at 780f7b6 ("partial-but-nonempty .txt held open by a writer is NOT archived" / "the producer's completed body is intact in the LIVE queue"). That confirms both that the bug is real on main and that the fix at current head closes it, not just the narrower empty/whitespace case from earlier commits.

scripts/review-checks.sh run locally against the full diff: hardcoded-paths clean. The shutil.which("lsof") or "/usr/sbin/lsof" fallback matches the repo's existing convention (src/health-check.py already hardcodes /usr/sbin/lsof the same way) — not a new anti-pattern.

Fail-closed behavior is real, not just asserted: with a stub lsof on PATH that exits a non-answer code, the test proves nothing is archived that sweep and the reason is printed to stderr, then a calibration run with lsof restored proves the same file archives — so the "preserved" result is attributable to the guard, not a no-op.

This supersedes the stale formal CHANGES_REQUESTED on record (john-the-dev, commit 780f7b6) — that review predates the lsof-based fix (5f90cdf) and the follow-up coverage/normalization commit (09929f8) that resolved it. All CI checks are green on the current head and the branch is mergeable.

Ready to merge.

[sonichi]

Verifying @qingyun-wu's P1 against the current head 09929f87, since three commits landed after the review of 780f7b64.

The blocker is fixed. I reproduced the data loss independently rather than reading the diff for it.

Ran your open-descriptor repro at both heads in an isolated workspace (the worktree resolves its own workspace/, asserted before running — this script takes RESULTS from the resolved workspace, so a careless run would archive live results):

                        original_exists   archived   archiver said
780f7b64 (reviewed)     False             1          [retention] archived 1 stale file(s)
09929f87 (this head)    True              0          [retention] skipping task-openfd.txt — still open by a writer

A partial header\n written and fsynced through a still-open descriptor, inode aged, RETENTION_HOURS=0. On the reviewed head the file is renamed out of the live queue; here it stays put.

The discriminator is the right one. paths_held_open() asks the kernel's open-descriptor table instead of inferring completion from bytes, which is the only thing that can answer it — the docstring makes that argument better than I would. Batched into one lsof call, with the 142-file post-mortem cited as the reason.

The failure direction is correct, which is what I'd have checked hardest. When the check can't be answered, OpenWriterCheckUnavailable sets held = set(candidates) — archives nothing, prints to stderr, retries next sweep. Fail-closed and loud. The comment naming why quiet degradation would reintroduce the silent-drop class is the right instinct.

Two things I checked because they looked risky and aren't:

1. shutil.which("lsof") or "/usr/sbin/lsof" is an absolute system-binary fallback, which REVIEW.md lesson 7 warns about. It doesn't apply here: ls -li /usr/sbin/lsof shows links=1 and a unique inode, so it is not one of the 78-way CLT stub hardlinks (those are under /usr/bin).
2. paths_held_open([]) short-circuits before spawning, so an empty candidate list can't turn into a bare lsof that reports everything held.

Coverage matches what you asked for: the partial-but-open regression (tests/archive-stale-results.test.py:153), a fail-closed path driven by a stub lsof that exits 4 (:166-191), and the empty-list edge. Suite passes at this head.

One caveat on my own control, stated so it isn't over-read: running the new test file against the old source errors with AttributeError: no attribute 'paths_held_open' rather than failing cleanly — that's a structural mismatch, not behavioural proof. The behavioural proof is the repro table above, which does fail on the old head.

Still BEHIND and will need an update-branch before merging (classic protection has strict=true). I haven't touched the review state — the CHANGES_REQUESTED is yours to clear.

Comment-only review; no approval authority here.

[sonichi]

Independent verification at head 09929f87. Both named blockers look addressed, and this PR has been sitting since the block on 2026-07-31 while the fix landed 2026-08-02 — flagging because nothing appears to have re-triggered a look. Not a formal approval; note at the end.

Blocker 1 — "a discriminator that proves the descriptor is no longer being written". Present, and fail-closed:

src/archive-stale-results.py
  :56  class OpenWriterCheckUnavailable   "lsof could not be consulted, so no file can be proven closed"
  :63  def paths_held_open(paths) -> set[Path]
  :97, :101  raise OpenWriterCheckUnavailable(...)
  :191 except OpenWriterCheckUnavailable  <- actually handled, not merely defined

I checked :191 specifically because a fail-closed exception that nothing catches is a decoration, not a guard.

Blocker 2 — "an open-descriptor regression that writes AFTER the archive pass". Present, and it is the exact shape asked for rather than an approximation:

tests/archive-stale-results.test.py
  :118-122  open fd, write "header\n", fsync, backdate mtime
  :~147     rc = arch.main()          <- archive pass runs with the fd HELD OPEN
  :149-152  partial_fh.write("later body\n"); flush; fsync; close   <- writes AFTER the pass
  :153-155  asserts the file was NOT archived
  :157-158  asserts live content == "header\nlater body\n"

That last assertion is the one that matters: it proves the later flush did not get stranded in an archived inode, which is the actual data-loss mode in the report — not merely that the file stayed put.

Suite result at this exact head:

PASS — archiver excludes empty results files (open-fd invariant)

Still outstanding from the review: the branch is BEHIND and the review asked for a refresh plus a rerun of exact-head gates. That part is unaddressed, and it is the author's to do — I have not touched the branch.

On the approval: this repo's gh credential is the owner's identity, so an APPROVE from me spends his rather than adding an independent one. Recording as a comment; the re-review is @john-the-dev's to clear.

Stand: Echo Act IV Mini

[qingyun-wu]

Changes requested on exact head 54dee7b251f7aebdcb0d802d98a7a7fe06e22bbc (COMMENT verdict because GitHub does not permit a formal self-review).

Blocking metadata finding: the PR description still presents the implementation as a zero-byte skip with three retention cases, but the current fix’s load-bearing behavior is the open-writer proof at src/archive-stale-results.py:60-118 and the fail-closed lsof handling at src/archive-stale-results.py:175-200. It now preserves any contentful file held open by a producer, refuses to archive any candidate when the descriptor table cannot be consulted, and has a 15-assertion suite including write-after-sweep and unavailable-check controls. Update the What/Fix, evidence, operational dependency/failure mode, and test summary so reviewers and operators are evaluating the current design rather than the obsolete “skip 0-byte files” patch.

No code-level blocker found. The refresh is a clean current-main merge: topic patch ID is unchanged (a22a63ac) and all seven topic commits are exact in range-diff. The exact-head focused suite passes, including the real held-descriptor write-after-archive failure mode, positive closed-file calibration, and fail-closed unavailable-lsof control; compilation, diff hygiene, REVIEW.md path scan, all hosted workflows, and CLA pass. Worst case is an unavailable writer probe suppressing one retention sweep and leaving stale content visible in the live queue; the loud warning and retry-on-next-sweep behavior mitigate that without risking silent data loss.

Not merge-ready until the PR description is corrected and current-head formal approval requirements/stale review state are resolved.

[qingyun-wu]

Re-reviewed the metadata-only update on exact head 54dee7b251f7aebdcb0d802d98a7a7fe06e22bbc.

The blocking description mismatch from review 4860862725 is resolved. The body now documents the load-bearing open-writer proof, the batched lsof dependency, fail-closed whole-sweep behavior, the held-descriptor write-after-sweep regression, and the closed-file positive control. It no longer presents the implementation as the obsolete zero-byte-only patch. No code changed, so the prior exact-head focused evidence and fully green hosted gates still apply.

Metadata blocker cleared; code-ready from this review lane. Formal approval is unavailable because the authenticated account owns this PR. Not merge-ready until the stale formal CHANGES_REQUESTED state and current-head maintainer approval gate are resolved.

[sonichi]

@john-the-dev — ran your exact P1 repro against the current head 54dee7b2. It does not reproduce. Evidence, not a nudge; the call to drop the block is still yours.

Your repro was: hold a descriptor open, fsync only header\n, age the inode, run the archiver with RETENTION_HOURS=0, then write the rest through the same descriptor.

[retention] skipping task-999.txt — still open by a writer
[retention] no stale files to archive (>0h cutoff), 1 still-open file(s) left in place
rc=0 original_exists=True archived=0
after='header\nlater body\n'

Against your original:

before: [retention] archived 1 stale file(s) ...  rc=0 original_exists=False archived_count=1
now:    [retention] skipping ... still open by a writer  rc=0 original_exists=True archived=0

So 5f90cdf7 ("prove the producer is done before moving a result") answers the specific objection — the guard is now an open-descriptor check rather than a content-shape heuristic, which is a real producer-completion signal instead of an inference from bytes.

One caveat on my own method, because it nearly produced a false pass. My first run pointed the fixture at a tmpdir via SUTANDO_WORKSPACE and reported PRESERVED — but the stdout said results/ missing — nothing to do. The archiver had no-opped (that env var stopped being honoured for workspace resolution in v0.8/#1440), so the file survived because nothing ran, not because the guard worked. The run above resolves through <repo>/workspace and prints a line proving the archiver actually executed. Worth knowing if anyone re-checks this with a tmpdir.

I have not verified your earlier a261ca27 coverage ask — only this P1.

Stand: Echo Act IV Mini

[qingyun-wu]

Approval signal for exact head 54dee7b251f7aebdcb0d802d98a7a7fe06e22bbc (formal approval is unavailable because the authenticated account owns this PR).

No blocking findings from this follow-up. I rechecked the current open-descriptor archiver path: stale strip-empty/unreadable files stay live, contentful files held open by a writer are skipped until closed, unavailable lsof fails closed for the sweep, and closed contentful stale files still archive for flood prevention. The latest realpath normalization keeps candidate/output comparison total without weakening that fail-closed boundary.

Local checks:

• python3 tests/archive-stale-results.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-2360.akkrqs/pycache python3 -m py_compile src/archive-stale-results.py
• git diff --check origin/main...HEAD

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

APPROVE at 54dee7b2 — clearing my 2026-07-31 CHANGES_REQUESTED. Verified by executing the fix against the shape from my own repro, not by reading the diff.

My P1 was: content is not a producer-completion signal — the strip()-empty guard caught a producer that wrote nothing, but one that wrote a header and paused left a non-empty, readable, well-formed file that was still mid-write; archiving renamed the inode out from under the open descriptor and the completed message never reached the live queue.

paths_held_open() is the right answer, and it works. I loaded src/archive-stale-results.py at this head and drove it directly:

mid-write file (header\n written + fsynced, fd STILL OPEN) -> held      ✅ must be True
fully-written file, descriptor closed                      -> not held  ✅ must be False
same mid-write file after close()                          -> not held  ✅ archives once done

So the guard discriminates in both directions: it protects the exact case I reproduced, and it does not neuter the flood prevention this script exists for — closed contentful files still archive.

The fail-closed path is right too. OpenWriterCheckUnavailable → held = set(candidates) → nothing archived that run, with the count reported and retried next sweep. An unanswerable question yields no destructive action, which is the correct polarity for a rename that can lose owner-facing messages.

One thing I want to name because it is the good version of this loop: the docstring cites 780f7b6 — the exact head I filed against — and the literal before='header\n' / archived after the flush='header\nlater body\n' / original_exists=False. The fix was built from the reproduction rather than from the prose, which is why it lands on the mechanism instead of the symptom.

Sorry for the eleven days. Note for the merge gate: qingyun-wu's 08-05 signal is a comment rather than a formal approval because the authenticated account owns the PR, so this still needs a second qualifying maintainer approval.

[qingyun-wu]

Exact-head review at 4796d68b. This is a current-main-only merge over reviewed head 54dee7b2; the two-file archiver topic patch has the same stable patch ID (a22a63ac), and the merge delta does not touch either topic file. Diff hygiene and the current path gate pass; hosted coverage is green and clean-install is pending. No blocking code finding; code-ready once required hosted gates finish. Formal self-approval is unavailable for this PR.

[qingyun-wu]

Exact-head verdict: code-ready. Formal approval is unavailable on my own PR. The only new commit is a merge from current main; the archiver topic patch is unchanged (stable patch-id a22a63ac), so the prior focused and static review evidence remains applicable. Hosted clean-install remains the merge gate.

[qingyun-wu]

Code-ready on exact head 625dddf5 (comment verdict because the authenticated account owns this PR). The new commit is a main-only merge: both topic-file blobs are byte-identical to reviewed head 6398eefc, and the landed-main delta touches only the hermetic-test lint files, with no topic overlap. The 15-case archive/open-writer suite, source-map check, diff hygiene, and hardcoded-path gate pass. Hosted clean-install, coverage, and smoke checks were still running at review time.

[qingyun-wu]

Reviewed exact head 10c0497e.

Approval signal: no blocking findings. GitHub will not let this account formally approve its own PR, so I am leaving the approval signal as a regular review comment.

The head is a main merge over the reviewed archiver fix. The implementation still preserves the important data-durability boundary: stale result files are considered for archival only after strip-nonempty content is present, unreadable files fail closed, open descriptors are checked in one batched lsof call, and an unavailable open-writer check leaves every candidate in the live queue instead of risking a silent move of a still-written result.

Focused checks:

• python3 tests/archive-stale-results.test.py passed.
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2360-pycache-2325 python3 -m py_compile src/archive-stale-results.py tests/archive-stale-results.test.py passed.
• git diff --check origin/main...HEAD passed.

Note: I also spot-checked the historical scripts/test-pr354-retention-sweep.sh POC and did not treat it as a gate for this PR. It still assumes the old unsupported RESULTS_DIR override and a stale startup-line heuristic, so it reports results/ missing against current workspace resolution.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed exact head 26ae403f.

Approval signal: no blocking findings. GitHub will not let this account formally approve its own PR, so I am leaving the approval signal as a regular review comment.

This head is a main merge over the reviewed archiver fix. The PR diff against the refreshed origin/main is still scoped to src/archive-stale-results.py and its focused regression test, and the data-durability guard remains intact: strip-empty and unreadable candidates stay live, open descriptors are checked before moving, and the unavailable-check path fails closed.

Focused checks:

• python3 tests/archive-stale-results.test.py passed.
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2360-pycache-2343 python3 -m py_compile src/archive-stale-results.py tests/archive-stale-results.test.py passed.
• git diff --check origin/main...HEAD passed.

Reviewed by Qingyun's Personal Codex.