fix(sync-health): the sync-health poll no longer takes — or orphans — the agent's index.lock (#2742) - #2797
Merged
Conversation
…c-health polling (#2742) Trinity Rule #1 — the requirements/architecture delta lands before the code. What the docs now say: - requirements/github.md §11.9 gains "Lock-free status read" and "Stuck-lock report, never a runtime delete", and the stale-lock hygiene bullet records that the boot reap now speaks. §11.8's SyncHealthService bullet names the leader lease AND its honest side effect (time-to-sync_failing ~90s → ~180s). - architecture/agent-lifecycle.md is the single home: why the status read took index.lock ~2x/min in every workspace, what replaced it, and why the runtime delete was cut on measurement rather than hardened. - architecture/agent-runtime.md gets a one-line pointer (no second home). - architecture/background-services.md's Sync Health row states the lease, its fail direction and the reason (a feed that raises sync_failing must not go dark when Redis does) — per that file's header rule. - feature-flows/git-sync-health.md: §1a announced boot reap + the observe-only runtime report with the three measurements that decide it; a new §2a for the agent status handler with the two distinct bounds (35s caller / 90s computation) and the ~130s child-budget arithmetic that replaces the doc's wrong "~30s worst case"; Files Touched, Testing, Operator Controls; and a Known Limitations rewrite that retires the "still blocks the event loop" bullet and replaces it with the residuals this change does NOT close. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main.py starts SyncHealthService in EVERY uvicorn worker and prod runs `--workers 2`, with no lease — unlike opqueue:leader (#1632), monitoring:leader (#1464), skills:sync:leader and canary:leader. So every git-enabled agent was asked for /api/git/status twice a minute, and each ask runs a 30s credentialed `git fetch origin` inside the container. `synchealth:leader` in the #1464 shape: SET NX EX, own-lease-only refresh, release from stop(), acquire + transition logs at the top of _poll_cycle with an early return for non-leaders. Two deliberate divergences from the copied shape: - Release is a single compare-and-delete EVAL, not GET-then-DEL. The non-atomic form lets a worker whose lease lapsed between the two calls delete a sibling's FRESH grant — two pollers for a whole cycle. EVAL is outside the -@dangerous categories denied to the backend/scheduler ACL users. - Fail direction is OPEN and stated in the docstring: Redis down means every worker polls, i.e. today's behaviour. Failing closed would darken the only feed that ever raises sync_failing, exactly when infra is already degraded. The lease has an honest side effect and it is named, not buried. db.upsert_sync_state INCREMENTS consecutive_failures per failed upsert, so it is NOT idempotent: two unleased workers reached sync_failing in ~90s, one leader takes ~180s. Asserted in TestLeaderLeaseAlertTiming — including the load-bearing fact itself (the same payload polled twice increments twice), so the test cannot pass against an idempotent upsert and prove nothing. Also here (same file, same subject): SYNC_HEALTH_POLL_INTERVAL_SECONDS, read at CALL time in the _maintenance_timeout_seconds shape, parse-guarded and positive-clamped, with the default DELIBERATELY unchanged at 60s — the AC is written as "the 60s poll" and the sampler evidence was taken there. poll_interval becomes a property so the module-level singleton (built at import) still honours the env; `is not None` rather than truthiness so the tests' poll_interval=0 keeps meaning "one cycle then exit". The `service` fixture now stubs get_breaker_redis -> None: with a local Redis it would otherwise leave a real 30s lease behind and the NEXT test's fresh service would lose the election and silently poll nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`git status --porcelain` takes .git/index.lock on EVERY invocation to refresh the index — whether or not a rewrite follows — for a window that scales with index size (0.6ms at one file, 12.8-15.4ms at 20k; 334-473ms measured in a real container). The backend polls /api/git/status every 60s for every git-enabled agent, so the platform was taking that lock ~2x/min in every workspace, outside _REPO_LOCK, racing the agent's own `git add`. `git --no-optional-locks status --porcelain` on exactly that one call site. Scoped deliberately: the auto-sync cycle's status (under the repo lock, right after `git add -A`, proceeding to commit) and the sync/pull bodies keep the plain form — they are lock-serialized and they WANT the refreshed stat cache. Flag form rather than GIT_OPTIONAL_LOCKS=0 because run_registered has no env= kwarg, the env form would silently change the mutating sites too, and only the argv is assertable in a test. Verified across a clean index, a stat-dirty index, core.untrackedCache, --untracked-files=all, an fsmonitor hook and a repo with no .git/index: zero lock sightings and zero rewrites in every case. The flag closes only the index-lock half, so every status child now routes through run_registered (#1595's seam): a sweep tick straddling the 30s `git fetch` still SIGKILLs it, and a killed fetch orphans FETCH_HEAD.lock / packed-refs.lock — which NO reaper covers, not even startup.sh's (its find is scoped to refs/ and logs/). run_registered accepts neither capture_output nor text, so both kwargs are DELETED at all ten sites; a name-only substitution would be a TypeError ten times over. Three of those ten are the shared helpers _compute_ahead_behind, _get_pull_branch and _persist_last_remote_sha, which are also reached from _conflict_response (the 409 arm of every locked endpoint), sync_to_github and pull_from_github. So the conversion sweep-registers three children on the MUTATING paths too. That is desirable — those are the children a repack-length operation exposes longest — but it is stated and pinned by a test rather than left for a reviewer to find. Also here, because the diff was already moving these lines: - remote_url is now unconditionally redact_url_userinfo'd. The old shape special-cased @github.com and returned everything else VERBATIM, so any non-github.com remote (GHES, GitLab, a host-rewritten origin) put a live `https://oauth2:<PAT>@host/...` in the response body — proxied unmodified by git_service.get_git_status to the UI and the MCP tool. ent#615 owns the broader class; this is the one line of it in this diff. - _read_sync_state_file gates on st_size (64 KiB) BEFORE reading. That file is fully agent-authored and merged wholesale, and the backend reads every agent concurrently once a minute, so an unbounded read_text() OOMs both sides. - The comment beside _REPO_LOCK now says what it does NOT exclude: the agent's own git and the backend's docker exec sites. Misreading that is what produced a plan to treat a successful non-blocking acquire as evidence of quiescence. _compute_git_status is extracted verbatim as one blocking callable (the handler calls it directly for now) so the next commit can put the coalescing in front of it without also moving the body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three callers converge on this one route — the backend's 60s sync-health poll, the UI git panel's own 60s poll while open, and the MCP get_git_status tool — and each computation runs a 30s `git fetch origin` against one repo. The issue observed two overlapping fetches 0.9s apart. A guard on the poller alone would not have satisfied AC2. Now the first caller starts the computation and every other caller awaits the SAME future. The slot is keyed on the resolved home path, check-and-set is atomic because there is no `await` between the test and the assignment, and the agent server is single-process (agent_server/main.py calls uvicorn.run(app, ...) with no workers=) — the BACKEND is not, which is what the Redis lease is for. Coalescing happens ON THE LOOP, and only the computation takes a thread. This is the load-bearing shape choice, not a style preference: asyncio.to_thread uses the loop's DEFAULT executor — min(32, cpu+4) = 6 threads on a 2-vCPU agent — and services/headless_executor.py records that ctx.terminate, auto-sync and pipe-close deliberately stay on that same pool. Parking followers there is the #2433 starvation class, where a burst of status callers stalls EXECUTION TERMINATION. Five callers must cost exactly one to_thread, and a test asserts it so the rejected shape cannot come back. Two bounds, deliberately distinct numbers for distinct things: - _STATUS_FOLLOWER_WAIT_SECONDS = 35 is a CALLER bound, at or just past the point every real client has already given up (poller 10s, git_service 30s); a longer wait can only produce work nobody awaits. Timeout is a 504. - _STATUS_LEADER_DEADLINE_SECONDS = 90 is a COMPUTATION bound. The child timeouts sum to ~130s nominal before run_registered's post-killpg drain, and a slow leader costs no follower threads but DOES hold the slot, so every caller in that window 504s. The docstring carries the arithmetic; the flow doc's old "~30s worst case" (which is where a 60s follower bound came from) is corrected in the docs commit. asyncio.shield is load-bearing: a follower that times out or disconnects must not cancel the computation everyone else is waiting on. And the future is ALWAYS resolved — to_thread -> run_in_executor -> _WorkItem.run catches BaseException and calls set_exception — so there is no hand-rolled set_result/set_exception pair and no "leader vanished" fallback. The comment says not to add one. `computed_at` admits what coalescing costs: a follower arriving at t=29s of a 30s leader run is served a 29-second-old snapshot, visible as "1 ahead" right after a successful push. Stamping the age is cheaper and more honest than claiming coalescing changes nothing. TTL caching was rejected — serving late followers a fresh run reintroduces the overlapping fetch AC2 forbids. No 409 on status by design: it is a read, and _with_repo_lock on it would make every poll a contended write and flap the agent `unreachable`. D5: test_1920's walk now covers docker/base-image/agent_server/ as well as src/backend — Invariant #5, "a guard that walks only one of the two trees is not a guard" (ent#314). Proven by planting an nx=True in an agent-server module and watching it fail. That tree gets no allowlist row on purpose: it issues no nx=True set and cannot (agents are not on the platform network, so Redis is unreachable from one), and #2742's coalescing is a different class entirely. Its one-home property is asserted directly instead, beside a meta-assertion that fails loudly if the agent-server root ever moves out from under the walk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lock (#2742) AC3 has two halves and only one of them was missing. The RECOVERY already existed and is provably safe: startup.sh reaps index.lock at container start, where "no git process is running" is definitional because the PID namespace is empty. What was missing was the OBSERVABLE half — the block was `rm -f`, silent whether or not it removed anything, so the one moment the platform reliably heals a wedge produced no evidence that it had. startup.sh now tests-then-removes, echoes a line naming each lock it cleared (Vector captures it), and drops ~/.trinity/lock-recovery.json. It also reaps index.lock under .git/modules/* and .git/worktrees/*, which NOTHING covered before — its find is scoped to refs/ and logs/ — so a submodule or linked-worktree wedge used to survive every restart. Proven by extracting the shipped block and running it against fixtures: /verify-local's agent stage boots local:test-echo, so startup.sh never executes there and trusting the stage would prove nothing. _record_lock_recovery folds that marker into sync_state.last_lock_recovery on the next status read, then deletes it so the episode is reported once rather than every minute forever. It goes through a dedicated metrics-only writer: _write_sync_state_file unconditionally stamps last_sync_at = now, which would mark a never-synced agent as freshly synced and turn its dashboard dot green. _index_lock_stuck REPORTS a currently-present lock and never unlinks it. The runtime delete was cut on measurement, not on taste: - st_size == 0 is the signature of a LIVE writer, not an abandoned one — git creates the lock with O_EXCL before walking the worktree and writes the new index into it only at the very end. Measured 100% of a healthy `git add -A`'s life: 3.9s on 450MB, 29s at 60k files, 155s under a clean filter. - st_mtime is stamped at create and never advances, so age measures the IN-FLIGHT OPERATION. At t=+130s a healthy add reads size=0 age=130s — both naive gates satisfied. - A wrong unlink is permanent and strictly worse than the wedge: git renames by PATH, so a second git's in-flight file gets promoted onto .git/index, the corrupting process exits rc=0 with empty stderr, and the 0-byte index is cleared by nothing — not the boot reap, not _reap_stale_git_litter, not `git reset`. - _REPO_LOCK would not have helped: it excludes this server's own auto-sync cycle and nothing else, not the agent's own `git add`, which is the premise of this issue. So detection is two-point inode stability — the same (st_ino, st_mtime_ns, st_size) unchanged across >=3 status reads spanning >=15 min — on time.monotonic(), which a forward NTP step or a live migration cannot move. The tunable is the sighting COUNT, not a wall-clock age. Three properties that are each a defect if dropped: - It resolves the REAL gitdir. `.git` is a FILE for a linked worktree and for a submodule, both creatable by the agent in one command, and assuming a directory makes the observer look where the lock provably is not. Candidates cover <gitdir>/index.lock, modules/*/index.lock and worktrees/*/index.lock. A symlinked .git is skipped outright. - It takes NO repo lock. An lstat needs no mutual exclusion, and holding _REPO_LOCK across the observation would make a status poll a brand-new source of 409 agent_busy on an operator's POST /api/git/sync. - It is wrapped end to end in its own except OSError. It runs inside _compute_git_status's try, whose tail is HTTPException(500), and _fetch_git_status treats any non-200 as None and writes nothing — so one EACCES would silently stop the agent's sync-health row advancing and sync_failing would never fire either. An observability path must never be able to darken the feed it feeds. The sighting ledger is in memory, deliberately NOT in sync-state (a deviation from the plan, on safety grounds): a per-tick read-modify-write into the agent-authored document would race the auto-sync writer and could drop consecutive_failures or last_sync_status — the observability path corrupting the feed. It needs no durability either, since the only thing that clears a genuinely wedged lock is the container restart that also clears the ledger. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backend half of AC3, plus the AC4 concurrency proofs.
_coerce_lock_recovery / _coerce_lock_stuck rebuild both new fields from values
the backend has checked, and never pass the agent's dict through. That is not
belt-and-braces: sync-state.json is agent-authored, the agent server merges it
wholesale (merged.update(data)), so last_lock_recovery is fully agent-controlled
even on an agent where nothing ever reaped anything — and
git_service.get_git_status proxies response.json() UNMODIFIED to the UI and the
MCP tool.
Three guards, each for a named failure:
- isinstance(value, str) before parsing: parse_iso_timestamp raises
AttributeError, not ValueError, on a non-str.
- except (ValueError, TypeError) around the parse AND the window comparison: a
valid-but-naive ISO parses cleanly and then raises TypeError on aware > naive,
and that raise lands in _sync_agent AFTER the upsert where
gather(return_exceptions=True) swallows it — so the symptom is a lost alert
with no traceback. The same guard with the same comment already exists in the
file this record comes from.
- An explicit UTC offset is REQUIRED. startup.sh always stamps Z, so a naive
value did not come from us, and the house posture over agent-authored JSON is
to reject rather than guess.
The window is [now - 1h, now + 60s], with the hour deliberately a floor rather
than 2x the poll interval: startup.sh stamps `at` at container boot and the
agent server folds it in on its FIRST status read, and a cold boot can put
several minutes between those two. The clamp's job is to reject nonsense — the
far-future `at` that would otherwise be "newer" forever — and the DEDUP is what
stops a real record repeating.
Dedup is against the last OBSERVED value. The rejected alternative ("at newer
than the prior row's last_check_at") is not a dedup at all: last_check_at is
re-stamped to now on EVERY upsert, so 9999-01-01 would be newer on every tick,
per agent, for the life of the process — a WARNING asserting a platform action
that never happened. Both halves are tested, so neither can pass by rejecting
everything.
index_lock_stuck is edge-triggered and re-arms after the lock clears. No
operator-queue item and no DB column: the report is a diagnosis, not a decision.
The agent-supplied `path` is dropped at the boundary — it is composed from a
.git the agent can point anywhere and adds nothing to a fleet-level WARNING.
AC4 ships in three phases, strongest first:
- The GATE is deterministic AND genuinely concurrent: a real
`git status --porcelain` child is SIGSTOP-frozen the instant it takes
.git/index.lock, and the agent's own `git add` — a second real process — then
fails rc!=0 with index.lock in stderr. 10/10 locally. SIGSTOP is the only way
to hold that window open: git runs hooks and filters OUTSIDE the index lock
(an fsmonitor hook sleeping 1s stretches status to 1279ms while the lock
window stays 0.8ms). SIGCONT is in a `finally` — --timeout-method=signal
raises inside the test and a leaked frozen child would hold the lock for the
rest of the session. Its twin proves the flagged argv cannot be caught at all.
- The lock-sighting sampler (AC1, earlier commit) is the property.
- The threaded witness through the real get_git_status() route is
self-validating and non-gating: its control arm must reproduce an index.lock
failure IN THIS RUN or the test skips. Measured through this route the duty
cycle is ~0.7% (about 95% of each iteration is git fetch), so the margin is
roughly one failure per run — it skips ~2 runs in 5 here, which is exactly the
point: it can never pass without having demonstrated it can fail.
Writer rounds write time.time_ns() and failures are classified by
"index.lock" in stderr, never by return code: `git commit` exits 1 with EMPTY
stderr when a round writes content identical to the previous one, which under
check=True is indistinguishable from a lock failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tail steps: /update-tests and /sync-feature-flows. Two gaps the plan's test matrix listed and the implementation had not yet covered, both now closed: - test_leader_deadline_releases_the_slot — a wedged leader must release the in-flight slot rather than monopolise the read. It holds no follower threads on this design, but it does hold the slot, so every caller in that window 504s. - An AST guard pinning _STATUS_LEADER_DEADLINE_SECONDS to the status body's REAL child-timeout budget. They are both 90s today (10 rev-parse + 10 status + 10 log + 30 fetch + 10 merge-base + 10 log + 10 remote get-url). Adding a child, or widening one, now fails here instead of showing up as an unexplained 504 in production — and the flow doc's arithmetic is guarded with it. Plus the ordering invariant between the two bounds (caller < computation). Flow-doc sync — one home per feature, so the two adjacent docs get pointers, not copies: - github-sync.md documents GET /api/git/status's own contract, so its endpoint row, its _persist_last_remote_sha row (whose child is now sweep-registered on the LOCKED sync path too), its stale "line 249-250" call-site reference, and its revision history all needed the #2742 delta. - mcp-git-tools.md: the MCP tool proxies the agent payload verbatim, so it gains computed_at / lock_recovery / index_lock_stuck with no signature change — and its callers now coalesce with the poller and the UI panel rather than stacking a third overlapping git fetch. - feature-flows.md's Git Sync Health category row and git-sync-health.md's index_lock_stuck field list corrected to match what the code actually emits. The test-runner catalog entry lives in the private .claude submodule and is committed THERE, on its own branch, deliberately WITHOUT bumping this repo's gitlink — `git add -A` stages that gitlink silently under `diff.ignoresubmodules=all`, which is what ejected #2606 from a merge train. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…est (#2742) Public repo. The fixture token was never real and is too short to match GitHub's own secret-scanning format, but a `ghp_` prefix is exactly what the repo's pre-commit checklist tells a reviewer to grep for, and the test's point is that URL userinfo is stripped — not that it is a GitHub PAT specifically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n pitfall (#2742) Moving the status handler off the event loop removed a mutual exclusion that sync_to_github/pull_from_github were providing by never awaiting. _REPO_LOCK was added for the cycle, so it does not cover a newly threaded path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nts (#2742) Re-measured inside a real agent container on the ext4 workspace volume. The penalty tracks content bytes re-hashed, not index size: ~1x on 42491 files of ~1 MB, ~390x on 1500 files of 294 MB. The single 29x figure was taken outside the fleet and sat between the two regimes, describing neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…2742) `SYNC_HEALTH_POLL_INTERVAL_SECONDS` was read by `sync_health_service` at call time — a property rather than an import-time copy, specifically so an operator override is honoured — and then reached no container: it was absent from `docker-compose.yml`, `docker-compose.prod.yml` and `.env.example` alike. The knob shipped inert. Proven by rendering rather than grepping: before, the var does not appear in `docker compose config`'s backend environment at all. This is the #1056 packaging class (`VOIP_*`), which `test_ent237_skill_source_env_packaging.py` records as having already recurred seven times. Prod compose launches standalone — no base-compose merge and no `env_file:` on the backend service — so the explicit `environment:` list is the only route in, and wiring dev alone would not have carried over. The `${VAR:-60}` pass-through form is the correct one here: a cadence has no "disable" sentinel, so unset and empty must both land on the unchanged 60 s default. `TestPollIntervalReachesTheContainer` asserts the FORM, not mere presence, and pins the compose default against `DEFAULT_POLL_INTERVAL` rather than a literal so the two cannot drift into a container that polls at a different rate from a laptop. Verified to have teeth by deleting the prod wiring and watching it go red. Found by /validate-pr §4.9 on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…2742) `b6757ba66` wired `SYNC_HEALTH_POLL_INTERVAL_SECONDS` into `docker-compose.yml`, `docker-compose.prod.yml` and `.env.example` — and missed `docker-compose.hosted.yml`, the pull-only twin every marketplace and managed-host channel actually runs. `test_2280_hosted_compose_parity` caught it in CI: deterministic, all three head seeds, clean in all three base seeds. That is the bug reproducing inside its own repair. The commit being fixed exists because a knob never reached the container; it then failed to reach the container that ships to marketplace installs. The hosted file's own header names the class and lists its five prior victims (#1039, #1056, #1707, #1871, #2381) — this is the sixth, and the first to be caught by the guard rather than by an operator. The line is byte-identical to prod's, since #2280 compares the backend `environment` list wholesale. No hosted-specific assertion is added here: that guard is strictly stronger than anything this file could restate, and two guards over one fact drift apart. The reasoning is recorded in the test's docstring so the next reader does not "helpfully" add the redundant one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndriiPasternak31
marked this pull request as ready for review
September 14, 2026 23:31
Contributor
|
merge-train: this PR is on today's train. Nothing was pushed to your branch and the body needed no patch. Validation found no critical. The fix is real and correctly scoped: Three things recorded, none blocking:
One merge-order note: #2804 (the test sweep) also edits |
This was referenced Sep 15, 2026
…e-2742 # Conflicts: # docs/memory/feature-flows/github-sync.md # docs/memory/learnings.md # tests/registry.json
vybe
approved these changes
Sep 15, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
merge-train: batch validated on train/20260915-1011 (#2808) — full suite green across all five members together; dev merged in for the learnings.md / github-sync.md appends and the registry.json rebuild (251 entries, both sides' rows kept).
3 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2742
The 60 s sync-health poller took
.git/index.lockin every git-enabled agentworkspace ~2×/min, outside
_REPO_LOCK, racing the agent's owngit add— and asweep-killed status child orphaned a 0-byte lock after which every later git
write failed silently (
git statusitself returns rc=0 against a stale lock —that is the "silent" in the title).
AC 1 — the 20 Hz sampler, before and after
Measured in a real agent container (ext4, 42 491 index entries), 120 s / 2400
samples per arm, on the image this branch builds.
lock_present/ samplesgit status --porcelaingit --no-optional-locks status --porcelainSyncHealthServicepollThe issue's own pre-fix baseline was
samples=2400 lock_present=16.Why arm A is not decoration. A bare
lock_present=0proves nothing on its own:the lock window scales with index size (0.6 ms at 1 file, ~13 ms at 20 k), the
sample period is 50 ms, and the real poller fires twice in 120 s — so an unfixed
build also frequently reports 0. The number only means something once (a) the
instrument is shown to catch the pre-fix argv on this workspace — 1083 times —
and (b) the poller is shown to have actually polled. Arm C's attribution comes from
two independent signals:
agent_sync_state.last_check_atadvanced 3 times exactly60 s apart (
22:43:17.720Z,22:44:17.881Z,22:45:17.973Z), and the agent's ownaccess log shows the matching
GET /api/git/status 200arriving from the backend.A vs B is the powered comparison — identical duty cycle, one variable. C is the
literal acceptance criterion.
AC4 is proven deterministically and in both directions by SIGSTOP-freezing a
real
git statuschild the instant the lock appears and running a genuinelyconcurrent
git add: unflagged 12/12 froze and 12/12 failed onindex.lock;flagged 0/12 and 0/12 — the flagged arm never freezes because the lock never
appears, which is simultaneously the AC1 proof. Reproduced 30/30 on the base
image's own git 2.39.5.
Three things a reviewer must not miss
1. The leader lease changes when an alert fires.
main.pystarts this service inevery uvicorn worker and prod runs
--workers 2. The newsynchealth:leaderleasemakes that one poller — but
upsert_sync_stateincrementsconsecutive_failuresper failed poll, so it is not idempotent: one poller instead of two moves
time-to-
sync_failingfrom ~90 s to ~180 s. Documented, tested(
TestLeaderLeaseAlertTiming), and said out loud here. The lease fails open —Redis down ⇒ every worker polls, i.e. exactly the pre-#2742 behaviour. Failing
closed was rejected: this feed is how
sync_failingis ever raised, and darkeningit precisely when infra is degraded is the worse error.
2. The runtime delete was cut on measurement, not taste. A 0-byte lock is the
signature of a live
git addfor ~100 % of its life (29 s at 60 000 files, 155 sunder a
cleanfilter), andst_mtimeis stamped at create and never advances — soneither size nor age separates abandoned from busy. Every sighting across all three
arms above was
size=0. A wrong unlink is permanent and worse than the wedge: gitrenames by path, so a second git's in-flight file is promoted onto
.git/indexandthe corrupting process exits rc=0, after which nothing in Trinity clears it. The
lock is therefore reported, never deleted (two-point inode stability,
gitdir-resolved,
lstat-only, no repo lock). Recovery stays where "nobody holdsthis lock" is provable for free — container boot — and the #1595 reap there now
announces what it cleared, surfacing as
sync_state.last_lock_recovery. Provenlive on a real orphan: the marker appeared with
last_sync_atstillnull, i.e.the recovery was recorded without faking a sync.
3.
--no-optional-locksis not free, and the cost is regime-dependent. The flagsuppresses the index writeback that also caches stat results. Re-measured in a real
agent container, the penalty tracks content bytes re-hashed, not file count:
~1× on 42 491 files / ~1 MB, but ~390× on 1 500 files / 294 MB (plain settles
to 0.001 s, flagged stays at 0.389 s). An earlier single "29×" figure sat between the
two regimes and described neither; it has been replaced in the flow doc. The obvious
git update-index --refreshmitigation was measured not to work (git'sracily-clean rule). AC1 is not negotiable and the flag is the only thing that
satisfies it, so this is accepted, quantified and documented — not hidden.
Scope
--no-optional-lockson exactly one call site(
_compute_git_status, the poller's read path; the auto-sync commit path and thesync/pullbodies want the writeback and keep the plain form). Ten git childrenroute through
run_registeredso a sweep tick during the 30 s fetch cannot orphanref litter. The handler becomes a loop-level single-flight over one
to_threadworker — coalescing on the loop rather than in
to_threadis load-bearing:followers blocking in the default executor is the bug: watchdog fails admitted-but-undispatched executions as "completed on agent but status not reported", releasing their slots and masking the real terminal #2433 starvation class.
SYNC_HEALTH_POLL_INTERVAL_SECONDSknob(default unchanged at 60 s), coerced+clamped agent-supplied
lock_recovery/index_lock_stuckbehind a 64 KiB read gate, andremote_urlunconditionallyredact_url_userinfo-d — the non-github.combranch was returning a fullytokenized URL in the response body.
test_1920's walk now covers the agent-server tree (Invariant Setup improvements #5: aguard that walks one of two trees is not a guard).
Validation
/validate-pr(Lane C) flagged one critical finding, fixed inb6757ba66:SYNC_HEALTH_POLL_INTERVAL_SECONDSwas read by the backend but wired into none ofdocker-compose.yml,docker-compose.prod.yml,.env.example— the knob shippedinert (the #1056 packaging class, its seventh recurrence). Proven by rendering
rather than grepping: absent from
docker compose config's backend environmentbefore, present after (dev honours an override, prod defaults to 60).
TestPollIntervalReachesTheContainernow pins the${VAR:-60}form, verified tohave teeth by deleting the prod wiring and watching it go red.
/verify-local8/8 PASS including the agent stage (unit15657 passed, 31 skipped, 0 failed; integration70 passed, 13 skipped). Affected unit files afterthe rebase: 127 passed, 1 skipped — the skip is the designed AC4 witness skip
(its control arm must reproduce a failure or skip; it can never pass without having
shown it can fail).
Known residuals, reported not fixed
FETCH_HEAD.lock/packed-refs.lockare covered by no reaper at all.Six follow-ups from the plan's Deferred Items are queued for filing (fetch-free
health poll, ref-lock litter, no
http.lowSpeedLimitin the base image,_reap_stale_git_litterfollow-stat,execute_command_in_containerdroppingtimeout=, consolidating the three lock-reaping policies).🤖 Generated with Claude Code