status: fix FSMonitor history and clean-proof gaps by ttaylorr-oai · Pull Request #74 · openai/git

ttaylorr-oai · 2026-08-26T17:25:19Z

Summary

reject missing clean-status snapshot paths before calling open(2)
serialize asynchronous simple-IPC start and stop transitions, make the
shutdown wake reliable across EINTR, and contain SIGPIPE during gentle
client writes
flush the Darwin FSEvents stream and drain its serial callback queue before
accepting a query boundary, with bounded timeout, daemon retirement, token
generation checks, and request coalescing

Controller scope

This review-only pull request presents one prerequisite and two topic patches
on top of 1293167e46. Approve it for controller admission, but do not merge
it into codex-unstable.

Performance

Overlapping Darwin queries whose cookies are already registered share one
provider fence. A timeout or shutdown intersection falls back conservatively
and retires the daemon. Paired same-base latency qualification is being run on
this exact source and binary; this description does not make a latency claim
before that evidence completes.

Validation

At commit 7bed8a334f:

clean build
t0052-simple-ipc.sh (11/11)
immediate-close client stress (200/200 on one socket path and 200/200 on
unique paths)
forced-EINTR shutdown wake stress (50/50, with 50 verified injections)
test and diff checks

The preceding candidate, 08f907411e, completed unit tests (406/406),
t7519-status-fsmonitor.sh (110/110),
t7527-builtin-fsmonitor.sh (194/194),
t7530-status-clean-sidecar.sh (59/59), and the
ASan+UBSan clean_status_manifest unit tests (4/4). The only tree change from
that candidate is replacing an open-coded allocation in
t/helper/test-simple-ipc.c with CALLOC_ARRAY() as required by static
analysis. Production sources, FSMonitor tests, and the FSMonitor topic
patch-id are unchanged.

Exact-head hosted CI, same-base latency qualification, and executable workflow
qualification are in progress.

with_lock__wait_for_cookie() gives a filesystem provider one second to report a synchronization cookie. A healthy FSEvents stream can miss that deadline while macOS is under load. The daemon then returns a trivial response, and status scans the entire index even though event delivery is still making progress. 4b1c56a (fsmonitor: flush pending FSEvents before cookie wait, 2026-07-21) requested an asynchronous flush on every Darwin query but kept the same one-second deadline. f439708 (Revert "fsmonitor: flush pending FSEvents before cookie wait", 2026-08-17) reverted it after a matched 48-query test still saw 12 timeouts in each arm. Avoid restoring that unqualified hot-path request. When the initial Darwin wait expires, request an asynchronous FSEvents flush and wait one more bounded interval. Successful queries retain the original wait and do not issue a flush or extend their deadline. The asynchronous call cannot block on the callback while the client holds main_lock. If the provider stays silent, retain the existing trivial-response fallback after the retry. Add a test-only callback delay to exercise both outcomes: a 1.2-second delay is recovered, while a 2.5-second delay still reaches the bounded fallback.

The daemon currently assumes that each client which advances an FSMonitor token also updates the repository's canonical index. That does not hold for commands using GIT_INDEX_FILE. A private index can advance the daemon past the canonical index's token and cause the canonical index's next query to receive a global invalidation. Keep a deduplicated overflow batch instead of discarding old paths. Clients at the overflow sequence still get an exact delta. Older clients get a conservative union of paths, which may overreport but cannot miss a change. All paths are interned. Keep a pointer-identity hash set with the overflow batch so later compactions hash only newly retired paths, rather than rebuilding a set over the daemon's lifetime history. Add a regression which advances a private index repeatedly, verifies that compaction remains deduplicated, and then checks that a read-only canonical status reports both changed files without a trivial response.

Retired batches are collapsed into a path-only overflow set. That keeps old indexes complete, but it loses the sequence in which each path was last observed. A client that consumed an inode event can therefore see it again after another index compacts the batch list, causing repeated hard-link scans. Unpinned batches have a zero pinned time and are also eligible for compaction immediately despite the default grace period. Do not use unpinned batches as truncation boundaries. Record the newest original batch sequence for every overflow path, and filter overflow responses against the client's requested sequence. The normal batch walk remains unchanged; sequence lookups are confined to overflow responses. Cover both the default retention grace and the cross-index hard-link case. The latter persists a nonzero checkpoint, compacts through a private index, and verifies repeated canonical reads do not rescan or fall back to global invalidation.

The delayed-cookie tests send the v1 timestamp token "0" and only check that the response is nonempty. Both recovery and fallback can satisfy that assertion with the same trivial response, so the tests do not distinguish a rescued cookie from a token-generation reset. Send a deterministic valid v2 token instead. Verify that the 1200ms case preserves its token generation without a global invalidation, while the 2500ms case changes generation and sends the fallback invalidation.

215845a (fsmonitor: preserve authenticated proofs across ordinary commands, 2026-08-15) enabled the clean-status history handoff for merges, but excluded invocations where fast_forward was FF_NO. Requested merge topology does not determine whether the resulting index is semantically safe. A clean non-fast-forward merge can carry the same authenticated FSUC/FSCF state as a fast-forward merge. As a result, --no-ff, --no-ff --no-commit, and merge.ff=false all dropped FSUC and reduced the FSCF flags from 15 to 9 after a clean merge. Each subsequent read-only status invalidated the external history and rescanned the semantic manifest. Enable the handoff for every merge using the canonical index. Conflict handling still invalidates unsafe proofs, and explicit alternate indexes remain excluded. Cover all three non-fast-forward forms, repeated read-only status calls, conflicts, and alternate indexes.

An exact clean status can repair a stale FSMonitor checkpoint or cached stat data while it scans. The repair requires an index write, so the existing issue path leaves no clean sidecar behind. Read-only callers then repeat the full scan until a second writable exact status publishes the proof. After the repair is written and resumable history is durable, install a sidecar bound to the rewritten index. Keep optional-lock-disabled commands read-only, preserve the literal exact-command restriction, and do not extend sidecar support to linked worktrees. Cover repeated read-only scans after a legacy daemon replacement, the single writable index repair in main and linked worktrees, and the next read-only sidecar hit in the main worktree. Keep option-bearing status commands ineligible for proof publication.

A configured pull can discard each layer of authenticated status history even when worktree inputs remain unchanged. Command-scoped protocol and HTTP settings change the config digest, directory events with more than 64 tracked descendants reject the semantic proof, and a fast-forward which adds an indexed directory drops the paired untracked cache. The next status can consequently preload and refresh the full index. Treat command-scoped protocol and HTTP settings as transport-only. For a large directory event, authenticate each distinct attribute source once instead of rejecting the cone outright. When a checkout adds tracked paths, retain the paired untracked cache and replay those additions through its existing invalidation path. Cover configured pulls in main and linked worktrees, large directory events, nested attribute-source changes, and branch switches which add tracked directories. The conservative full-scan fallback remains in place when an attribute source changes.

Configured pulls preserve FSMonitor clean proofs when checkout can authenticate every index change. Tracked policy files were an exception: adding or replacing .gitattributes or .gitignore made the generic semantic transfer reject the whole proof. Later read-only status commands then had to rescan the worktree and could not restore the paired untracked proof. Let checkout retain history across regular policy-file changes that it writes itself. Attribute changes refresh the worktree manifest before the provider boundary is rebound, and fail closed if that refresh cannot authenticate the new sources. Keep the existing untracked-cache invalidation for ignore changes, and transfer that cache only while the full tracked proof remains current. Exercise configured fast-forward pulls in main and linked worktrees. A required-filter control also verifies that changed attributes invalidate the affected tracked entry instead of certifying it.

A clean status proof can survive a pull only when its configuration, tracked-file state, FSMonitor token, and paired untracked cache still describe the resulting worktree. Command-scoped push transport settings were included in the configuration fingerprint. Checkout could also discard the untracked proof for policy-file changes or leave events from its own worktree writes outside the proof. The next diff, write-tree, or status then repeated tracked and untracked work. With optional locks disabled, status could not publish the repair, so each invocation paid the same cost. Treat push.negotiate and remote.*.pushurl like other command-scoped transport settings. Preserve the paired untracked cache across checkout, invalidate only affected policy scopes, and authenticate distinct attribute-source directories before transferring semantic history. For checkout, reset, merge, and sequencer worktree updates, write a provisional index under the existing lock, consume the daemon events caused by the update, and certify the result against that locked index before the final write. This also covers stash cleanup through its hard reset. Alternate indexes, split or sparse indexes, unsafe filter or manifest state, and incomplete stat data still fall back. Cover configured pulls, root and nested policy changes, main and linked worktrees, rebase, reset, stash, checkout, and repeated read-only status.

The Linux listener queued every inotify event as a file pathname. Directory events therefore lacked the trailing slash used by semantic invalidation. After an owned worktree update retained a clean proof, a following read-only status could not close those events against it. The command scanned all tracked entries. With core.preloadIndexBulk enabled, this work appears as statx calls instead of lstat counters. Format Linux worktree events through fsmonitor_format_worktree_paths() and use IN_ISDIR to preserve their directory identity. Advertise directory metadata support and mark Linux tokens so clients replace daemons using the old event format. Concurrent clients can see an expected connection reset while one client replaces a stale daemon. Silence that diagnostic only for gentle IPC reads, which already reconnect, without changing ordinary IPC error handling. Cover both bulk preload modes plus single and concurrent daemon replacement.

Bulk index preload can defer content and conversion checks to the diff that normally follows refresh_index(). repair_fsmonitor_proof() only refreshes the index before deciding whether to persist a clean proof; it does not run that diff. With core.preloadIndexBulk enabled, a pull or rebase that changes .gitattributes or .gitignore can therefore leave tracked entries dirty after the writer reports a successful repair. Repeated read-only status calls cannot persist the missing repairs. Do not request deferred bulk results in the writer-repair path. This keeps the ordinary status and diff bulk path unchanged while forcing the exceptional repair to finish its tracked checks before certifying and writing the proof. Enable bulk preload in the existing fast-forward, policy-file, and sequencer writer tests. They verify targeted refreshes and two subsequent read-only status calls without scans or index writes.

A writable Git command can leave a complete FSMonitor proof in a repairable state when it changes policy files, adds or removes an intent-to-add entry, or delegates the final index write to a child process. The existing repair path assumed that the manifest and untracked cache remained closed. The sequencer also kept its stale in-memory index after git commit rewrote the canonical index. Stash operations and completed rebases could therefore drop FSUC or overwrite the child's newer token. Read-only status could not persist the repair and repeated tracked or directory work. Let index-only writers refresh changed manifests and rebuild the paired untracked cache against the provisional locked index. Preserve unrelated history for safe intent-to-add changes and unmerged non-attribute paths, then reload the canonical index after child writers before repairing it. Active filters and unresolved structural indexes still fall back. Linux can report an event for a watched directory without a child name. Keep the watched directory in that case, encode its token capabilities in the order understood by Linux clients, and serialize incompatible daemon replacement on Linux as on macOS. Cover stash creation and application, policy-file updates, ordinary and --rebase-merges conflict completion, cherry-pick's deliberately weaker tracked-only proof, nameless inotify events, and primary and linked worktrees. Repeated optional-lock-free status calls must not rewrite the index or rescan tracked entries.

snapshot_open() initializes its output before passing the path to open_nofollow(). The manifest builder can ask it to pin a synthetic repository while rejecting a sparse index. Such a repository need not have an index path. Ordinary Linux happened to return EFAULT from open(NULL), but passing NULL violates the contract of open(2) and aborts under UBSan. Reject NULL and empty paths after initializing the snapshot. The existing clean-status-manifest sparse-index test exercises the fail-closed result.

766fce6 (simple-ipc: split async server initialization and running, 2024-10-08) separated server initialization from startup so owners could finish setup before accepting clients. But start and stop still inspect lifecycle flags without shared synchronization. A late start can therefore release workers after cleanup requested shutdown, and concurrent cleanup paths can repeat the stop sequence. Two transport races compound that problem during daemon replacement. An interrupted shutdown wake can publish the shutdown transition without waking accept(), leaving the stop path hung. A client whose accepted socket is closed before its request write can die from SIGPIPE before the gentle EPIPE recovery runs. Serialize the first start and stop, queue the complete shutdown wake before publishing that transition, and contain SIGPIPE within gentle client writes while preserving the caller's signal state. Exercise late startup, repeated stop, interrupted wakeups, and concurrent writes to a peer that closes immediately after accept().

The Darwin daemon treats delivery of its cookie-file event as proof that all earlier worktree changes have been published. That assumes the callback containing the cookie cannot overtake logically older work. A retained FSEvents trace disproves that assumption. The cookie callback completed before a later callback published removals that had happened before the cookie was created. A status query could therefore answer from incomplete event history and report a dirty worktree as clean. After the ordinary cookie wait, ask a long-lived worker to flush the FSEvents stream and then drain its serial callback queue. The flush schedules provider events; the queue drain waits for those callbacks to finish publishing. Accept the boundary only when the cookie was seen in the same token generation, and coalesce overlapping requests onto a single fence. If the bounded fence times out or intersects shutdown, return a conservative result and retire the daemon before unsafe stream teardown. Advertise the stronger boundary as a capability and token suffix so new clients replace unfenced daemons while older clients retain prefix compatibility. Exercise split and blocked callbacks, timeout replacement, generation reset, listener shutdown, concurrent coalescing, second-wave requests, rename and cache scopes, and protocol compatibility. Keep status proof tests outside the split-index matrix where that proof is deliberately disabled, and materialize externally restored tokens before raw-index helpers consume them. The provider fence adds work to each Darwin query, while overlapping queries share a fence when their cookies are already registered.

ae161e8 (fsmonitor: validate builtin daemon responses before applying them, 2026-07-10) validates each worktree path with verify_path(). That helper enforces index-entry rules and rejects a .git component anywhere in a path. Filesystem providers can legitimately report such a component for an untracked nested repository. The client therefore rejects the entire response after an event such as scratch/.git/file, forcing a full worktree scan. Commands that need a current provider boundary cannot persist a clean proof from that query. Validate the narrower daemon-response contract instead: require a relative path with nonempty, non-dot components and at most one trailing separator. Keep rejecting absolute and traversal paths, but allow .git components that already exist in the worktree. Cover the parser directly and exercise a real Linux daemon event against an optional-lock-free status oracle.

fbd7a23 (rebase: introduce and use pseudo-ref REBASE_HEAD, 2018-02-11) records the commit currently being replayed. The sequencer normally deletes that ref before executing each todo item. When the final item stops for a conflict, rebase --continue commits the resolved result. pick_commits() then reaches the end of the list without entering another iteration and removes the rebase state directly. The merge backend skips finish_rebase() because the sequencer owns cleanup, so REBASE_HEAD survives a successful rebase. The same path also strands the fsmonitor proof when index.skipHash is enabled. After committing the resolution, the sequencer reloads the canonical index and repairs its proof through a close-only index.lock witness. A skip-hash witness has a null trailer and a fresh file identity, so proof-epoch validation cannot bind it to the in-memory index. Rebase succeeds without FSUC, and read-only status cannot repair it. Delete REBASE_HEAD whenever interactive-rebase state is removed. This matches finish_rebase() cleanup and also avoids retaining a ref for an explicitly quit operation. Propagate a failed deletion so rebase does not report success after leaving the stale ref behind. Give only PROVISIONAL_LOCK witnesses a real checksum. The final index rewrite continues to honor index.skipHash, preserving the normal index write fast path while giving proof repair an authenticated epoch. Extend the final-conflict test to require REBASE_HEAD during resolution, its removal after completion, and a reported failure when the ref cannot be deleted. Exercise skip-hash proof repair after a clean-prefix, conflicted replay in primary and linked worktrees, with plain and configured-filter repositories.

A proof repair can close one provider token, collect untracked results, reopen the token, and close it again with the same struct wt_status. If the first closure published untracked output, the second closure tries to publish another snapshot over it and hits: BUG: publishing untracked results over collected status This is reachable from stash pop when an index writer repairs a complete FSMonitor proof while an untracked path is present. Before closing a required new token, discard output explicitly marked as coming from an earlier authenticated token closure or bulk preload. Keep the BUG for ordinary caller-collected results, which must not be silently overwritten. Extend refresh invalidation to discard both authenticated forms as well. Allow the scripted provider to opt into proof repair, and add a regression covering the two-token stash path with visible untracked output.

An fsmonitor provider can reset while merge is reading an index with an authenticated clean-status proof. The reset leaves that proof available for revalidation, but merge updates the worktree before repairing it. The resulting index can lose FSUC after a clean merge. Read-only status cannot persist the missing proof, so every later status falls back. A multi-strategy merge can lose the same history after preparation. An external strategy may replace the index before it declines or reports a conflict. restore_state() then reloads that index while rewinding the worktree. A later built-in strategy sees the original repair decision, but no longer has the paired proof from which to repair. Resolved conflicts expose a separate instance of the same failure. merge clears the resolve-undo extension before updating the worktree. That removal sets RESOLVE_UNDO_CHANGED, which prevented checkout from transferring an otherwise current proof. The result had neither a live provider token nor a pending token from which the writer could repair. Revalidate an authenticated proof before a non-fast-forward merge updates the worktree. Repair it before built-in results are published, after successful external strategies, and after each restore_state() rewind. Permit transfer after the resolve-undo map has been cleared, since removing that optional extension changes neither tracked entries nor worktree contents. Continue rejecting a live resolve-undo map. A repaired writer stats only entries that lack provider validation or stat data before certifying the new index. Fast-forward merges retain their existing path, while conflicts continue to fail closed. Cover built-in ort with and without retained resolve-undo history, trivial and content-level resolve merges, an external strategy that declines, and one that leaves a three-stage conflict before a clean ort retry. Verify that clean results remove resolve-undo data, keep a paired proof, and leave repeated read-only status unable to rewrite the index.

ttaylorr-oai requested a review from dreynaud-oai August 26, 2026 17:25

ttaylorr-oai marked this pull request as draft August 26, 2026 18:53

ttaylorr-oai removed the request for review from dreynaud-oai August 26, 2026 18:53

ttaylorr-oai marked this pull request as ready for review August 26, 2026 19:17

ttaylorr-oai requested a review from dreynaud-oai August 26, 2026 19:17

ttaylorr-oai force-pushed the tb/codex/fsmonitor-hardlink-inodes-unstable branch from f5f8eaf to 0a572e5 Compare August 27, 2026 04:33

ttaylorr-oai added 12 commits August 27, 2026 12:12

ttaylorr-oai force-pushed the tb/codex/fsmonitor-hardlink-inodes-unstable branch from 0710046 to 1ecb949 Compare August 27, 2026 21:04

ttaylorr-oai force-pushed the tb/codex/fsmonitor-hardlink-inodes-unstable branch from 1ecb949 to 08f9074 Compare August 27, 2026 22:24

ttaylorr-oai added 2 commits August 27, 2026 17:36

ttaylorr-oai force-pushed the tb/codex/fsmonitor-hardlink-inodes-unstable branch from 08f9074 to 7bed8a3 Compare August 27, 2026 22:40

ttaylorr-oai force-pushed the tb/codex/fsmonitor-hardlink-inodes-unstable branch 2 times, most recently from ad39dc8 to 2ebd9f8 Compare August 28, 2026 05:07

ttaylorr-oai force-pushed the tb/codex/fsmonitor-hardlink-inodes-unstable branch from 2ebd9f8 to 16d98d8 Compare August 28, 2026 05:34

Navigation Menu

Uh oh!

status: fix FSMonitor history and clean-proof gaps - #74

status: fix FSMonitor history and clean-proof gaps#74
ttaylorr-oai wants to merge 19 commits into
codex-unstablefrom
tb/codex/fsmonitor-hardlink-inodes-unstable

ttaylorr-oai commented Aug 26, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ttaylorr-oai commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Controller scope

Performance

Validation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ttaylorr-oai commented Aug 26, 2026 •

edited

Loading