Repository navigation
fix(quarantine): select Windows rename before probing libc - #5963
elCaptnCode wants to merge 6 commits into
Conversation
liususan091219
left a comment
There was a problem hiding this comment.
Code review found no blocking issue; not approving yet because there is no approval from another reviewer and the required Python CI shard is red.
What I checked at 9951365:
src/undelivered_quarantine.py:67-68: thewin32early return now runs beforectypes.CDLL(None, ...), so Windows no longer reaches the libc probe that raisesTypeError. Linux/macOS order (renameat2 / renamex_np, then"none") is unchanged; only the deadwin32branch after the probe was removed.- The vendored copy
packages/ag2-sparrow/ag2_sparrow/undelivered_quarantine.pyis byte-identical (same blob 8c52fbb1f4). tests/quarantine-restore-no-clobber.test.py:27-32loads the production module and calls the real_probe_rename()withsys.platformpatched andCDLLset to fail if touched. That exercises the shipped function, not a copy.- Sibling search:
CDLL(Noneappears only in this module (plus its vendored copy) and in a shell script underskills/claude-gemini, which is not on the gateway import path. - Scope is honest: the PR body says the gateway still fails later on the
fcntlimport intask_hold.py. - CLA is green.
Still missing:
python standalone tests (4/9)failed inpool-restart.test.py::test_explicit_resume_then_restart_keeps_a_and_its_recorded_folder, which this PR does not touch. A maintainer needs to rerun it.- Approval from a reviewer with Windows context.
— Sutando (susans-MacBook-Pro)
9951365 to
76179d2
Compare
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
76179d2 to
210535d
Compare
210535d to
32870d1
Compare
The directory lock only covers opening and acquiring the key lock. Cleanup can legally obtain it while the publisher still holds the key and skip that sweep. Require a later post-join sweep to remove the orphan, retaining inode, single-open, and missing-directory-lock controls. (cherry picked from commit ffebe7c)
(cherry picked from commit 1e82e11)
(cherry picked from commit 84cdfc1)
(cherry picked from commit 478ddf5)
(cherry picked from commit 22da022137580c69765974266f750ebc071e014c)
|
@cla-assistant check |
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at 6bd7191a. No blocking finding; not approving yet because the approval bar is not met.
Checked
src/undelivered_quarantine.py_probe_rename(): thewin32branch now returns("os.rename", None)beforectypes.CDLL(None), which is what raisedTypeErroron Windows. Linux/macOS order (libc probe, thenrenameat2/renamex_np) is unchanged. The Sparrow copy carries the same change.tests/quarantine-restore-no-clobber.test.pyadds a behavioural test: withsys.platformpatched towin32andctypes.CDLLset to raise, the real_probe_rename()returnsos.renameand never loads libc.- CI green at this head;
license/clais success.
Not blocking
- The production change is 4 lines, but the diff is +80/-16 over 10 files because it also carries the test prerequisites from #5999, #6003, #6005 and a notifier fixture fix. Landing those first will shrink this to its own change, which makes it easier to approve.
Missing for approval: no approval at this head. It needs one substantive approval from a reviewer with Windows context.
— Sutando (susans-MacBook-Pro)
liususan091219
left a comment
There was a problem hiding this comment.
Commenting at 6bd7191a. No blocking code finding; not approvable now because it conflicts with main.
Checked
_probe_rename()insrc/undelivered_quarantine.pyand the Sparrow copy now returns("os.rename", None)onwin32beforectypes.CDLL(None), which is what raisedTypeErroron Windows. The Linux/macOS order is unchanged.RenamePrimitiveProbecalls the real_probe_rename()withsys.platformpatched towin32andctypes.CDLLset to raise, and asserts libc is never loaded. Behavioural, not a source check.license/clasuccess (1 context); no unresolved threads; added-line hardcoded-path scan 0 hits.
Missing
- Conflicts with
main(mergeable: CONFLICTING). It needs a merge or rebase ofmainand a fresh CI run. The diff also carries test prerequisites from #5999 (still open) and #6003/#6005 (closed without merging); when refreshing, please say which of those hunksmainalready covers and drop them, so the PR shrinks toward its own 4-line change. - No approval at any head. It needs one substantive approval, ideally from a reviewer with Windows context.
— Sutando (susans-MacBook-Pro)
Windows Python raises
TypeErroratctypes.CDLL(None)while importing the quarantine helper, before the existing Windows rename branch can run. Selectos.renamebefore probing libc; Windows keeps its native no-clobber rename behavior. The canonical helper and generated Sparrow copy stay in sync.Found while running the native baseline for #5499. This fixes that import failure only: the full gateway subsequently encounters an unrelated
fcntlimport intask_hold.py, so this PR does not claim the gateway now works on Windows.Validation on Windows 11 build 26300 / Python 3.13.7:
TypeError; updated source imports successfully.python tests/quarantine-restore-no-clobber.test.py -v: 13 passed, including occupied destinations and races.Cross-platform validation: the same 13 tests and 76 gateway-disposal/delegation tests pass on WSL Ubuntu/Python 3.12. Source synchronization and the committed-diff repository scanner pass. No deployed services or runtime state changed.
Reviewer focus: the Linux/macOS probe order and no-clobber contract remain intact. @thinkall
CI follow-up (2026-10-29): rebased the original Windows fix unchanged onto main
d68157f5e. The refreshed run at32870d180passed the package gate but failed four inherited fixture assumptions in shards 7 and 8. Current head6bd7191aeincludes explicit test prerequisites, each in its own provenance-marked commit:6bd7191ae: the serial-notifier fake tmux now exposes its staged prompt through the real pane parser. Its original five-second deadline, ordering and result checks remain; an added guard records premature second submission. With the same 0.12-second capture latency, the old fixture times out and the corrected fixture passes. This demonstrates the legal timing mechanism, not the historical runner's precise schedule.Only the original quarantine helper change affects production. The fixture prerequisites are separately committed for review; the full diff includes them until they land upstream. Documentation: N/A for these fixture corrections.
At exact head
6bd7191ae, all 13 quarantine tests passed on native Windows and Linux, plus 76 gateway disposal/delegation tests on Linux. Source synchronization and documentation audit pass. The affected ack and watcher suites, 18 pool tests, four worker-config tests, the delivery/restart journey, and the notifier suite pass (84 tests collected; one platform skip). The forced held-publisher schedule and full committed-diff scanner also pass. Local tests used isolated workspaces; the delivery suite uses the real fswatch dependency, as hosted CI does. CI run 40822689881 passed at this exact head: all 21 main jobs succeeded, including all nine Python shards, the Sparrow package gate and diff coverage. All reported jobs in that run passed or were skipped.The separate Windows runtime workflow remains branch/label gated; native local evidence is distinct from hosted Windows CI. The full gateway still encounters the unrelated
fcntldependency described above. No deployed service or runtime state changed. Review approval remains separate from CI.Current status (2026-10-30): latest CI run 40824320636 passed at unchanged head
6bd7191ae, and the raw commit status forlicense/clais success. Required review is still outstanding, including approval from a reviewer with Windows context. Hosted Windows runtime testing remains skipped; the native local results above do not replace that run. This PR is not merge-ready.