Skip to content

fix: close failed focus checkpoints before verified cleanup - #285

Merged
codeasier merged 11 commits into
mainfrom
fix/focus-checkpoint-cleanup-277
Nov 1, 2026
Merged

codeasier merged 11 commits into
mainfrom
fix/focus-checkpoint-cleanup-277

Conversation

@codeasier

@codeasier codeasier commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

Separate minimal runtime follow-up

This is the separately authorized runtime repair for the Windows defect exposed by #284; it is not added to the original tests-only PR. It changes only FocusService._checkpoint_file and adds a dedicated regression file (two-file delta against parent 5b6cbca2514d7c2f8ff059c83622ca479c3e12e2).

For early combined CI this branch temporarily includes the unchanged reviewed #281/#282 test stack. Merge sequencing: #281, then #282, then this PR. This is integration ordering, not a functional dependency of focus cleanup. Once the earlier PRs merge, the net main diff is these two files. All actual required checks and branch protections still apply.

Defect and repair

Windows CI evidence: a zero write failed before publication but left .focus.pt-snap-*.recovery, because unlink ran while the checkpoint descriptor was still open.

  • Attempt descriptor closure exactly once before failure cleanup; never retry a failed close against a possibly reused descriptor.
  • Recheck the existing safe-path and device/inode identity immediately before unlink; substituted paths are preserved.
  • Close on fstat failure but never delete a path whose identity could not be established.
  • Preserve the original error if close/cleanup raises OSError. Keep successful commit/rollback behavior. This does not claim atomic elimination of the check/unlink race.

Verification

Independent source review found no blocker. Exact reviewed source/test blobs are 054e95992aabe05216ed1acaca0c8f84a502ad07 / ed1a10666b80c408c507508242effbb5506ddaca. AST, Ruff, Black and diff checks pass; normal tests/build run only in exact-head CI.

16 portable cases cover zero/short-zero/write/permission/fsync/verify failure, short-write commit and rollback, secondary cleanup errors, close failure, missing identity, real replacement identity and symlink metadata. Real test-owned files/fds enforce close-before-unlink on every platform; no skips/xfails or warning suppression. The original #284 failure assertion remains unchanged and must pass on its final combined head.

No parser-v3 decision, schema/CLI behavior change, runner/security/network/credential change, tag/release/deployment or live-provider action. Refs #277. The tracking issue must remain open until #284 and its post-merge acceptance are complete.

Copy link
Copy Markdown
Owner Author

First CI pass found one test-double scoping error, not a new platform failure: test_unknown_checkpoint_identity_closes_without_unverified_deletion replaced process-global os.fstat and intercepted an unrelated fd (Linux job 124878364575, Windows job 124878364245).

Commit 203950ad5a706284d05fd2ee1d9d480c5a00055e limits injection to the owned checkpoint descriptor and transaction context, delegating other descriptors to the original function. Runtime is unchanged. Original-error identity, exactly-once close, no deletion without verified identity, and unchanged focus bytes remain asserted. Independent review and static checks passed; exact-head CI is now pending. No checks or assertions were relaxed.

Copy link
Copy Markdown
Owner Author

The reviewed repair head 203950ad5a706284d05fd2ee1d9d480c5a00055e passed all 12 applicable Linux/macOS/Windows checks (Test 41318220383 / Platform 41318220380), and #284's unchanged original Windows zero-checkpoint safety assertion also passed at its intermediate combined head.

After #282 merged, this branch now includes actual main e347be302632b328f29da212d65b062ec3f457ae through verified zero-file-delta normal merge 33c9f00b7767fb8f014a5f5d5b698f7e17be42fd. The source/test tree is unchanged, but new exact-head CI is still required. Merge waits for that and #282 post-merge CI. The focus-only scope uses the full ordinary matrix, actual installed-wheel CLI acceptance and fault/safety regressions; it does not claim nightly dataset evidence or waive such acceptance for unrelated runtime changes.

@codeasier
codeasier marked this pull request as ready for review November 1, 2026 16:22
@codeasier
codeasier merged commit e786bfc into main Nov 1, 2026
13 checks passed

Copy link
Copy Markdown
Owner Author

Final actual-main head 33c9f00b7767fb8f014a5f5d5b698f7e17be42fd passed all 12 applicable checks: Test 41322927437, Platform 41322927496. After independent review and final base/head/review/check rechecks, normal squash merge is e786bfc0395a035670cfa12d568022c758ec848a; its tree exactly matches the reviewed/tested repair. Post-merge CI is pending. This does not close #277: the final isolated tests-only #284 still needs its exact-head and post-merge gates.

Copy link
Copy Markdown
Owner Author

Post-merge validation succeeded at main e786bfc0395a035670cfa12d568022c758ec848a: Test 41325988806 and Platform 41325988854, all 12 applicable checks including every Windows shard and macOS. The minimal focus cleanup repair is fully delivered. #277 remains open only for the final isolated coverage-test PR #284 and its post-merge validation; no nightly dataset result is implied by this focus-only delivery.

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.

1 participant