fix(#894): detect systemd-supervised sync engines (local patch, upstream PR #1006 pending) - #4
Merged
Merged
Conversation
…as waited for (fujibee#984) (fujibee#999) * test: count a role's bridges by pair set, and return the value that was waited for (fujibee#984) The five fujibee#937 reap tests each waited on one observation and asserted on a different, later one. The waiter polled until the count matched, then counted again and returned that second reading -- and between the two the reaper kills the orphan and the launcher respawns, so the count passes through 2 and through 0. The number the caller got was one that nothing had ever waited for. Verification measured the same shape in isolation: with a 60ms observation the old form returns 2 and fails, the single-observation form returns 1 and passes. One site had a second defect on top of that. The pair-superset test spawns its own {alice,bob} bridge, and the counter matched the role name anywhere in a process's argv, so it answered 1 for that fixture: the `-ge 1` gate opened on the test's own fixture, before the launcher had spawned anything. Both are replaced together, at all five sites: _count_exact_role_bridges requires a pair set of exactly {name} -- the same set inequality the superset test's `kill -0` already relies on _wait_exact_role_count returns the value that satisfied the wait The argv-match pair had no other users in the tree, so it is removed rather than left standing beside the new one. A leftover second form is how four of the five sites came to be missed in the first place; with it gone, a missed site is a grep. The expected value stays 1, and is now reached for the stated reason: the reaper kills the pidfile-less orphan and the launcher respawns one bridge for the role, so exactly one bridge with pair set {alice} exists once it settles. In the superset test the {alice,bob} fixture is not one of them, which is what the `kill -0` on the next line asserts separately. The new test pins the counter itself. Its positive control comes first, because three zeroes are also what a counter that answers 0 to everything produces -- which is what the first version of this predicate did: `ps` renders a pair's tab separator as the four characters \011, three of them digits, so a single-character [^0-9A-Za-z] separator class matched nothing at all. The test also asks the waiter for a count it will never reach, which is the only assertion in the file that goes red if the waiter is ever rewritten to return its `want` instead of what it saw; every other check here passes under that rewrite. Whether this is what fires in CI is NOT measured. fujibee#984 records two failing tests on main and both of them are covered, but the failure does not reproduce on this machine, so a green shard after this lands is a green shard -- not a cause. * test: make the reap gate load-bearing, and prove an exhausted one fails (fujibee#984) The gate the five fujibee#937 reap tests open before making an orphan was a `for i in {1..80}; do ... && break; done` loop. Running out of tries and reaching the count leave the same state behind, so from the next line the two are indistinguishable. A test whose launcher had not spawned yet went on to `rm -f` pidfiles that did not exist, waited, and was then satisfied by a bridge the launcher started during that wait -- green, having created no orphan and reaped none. The test passes without exercising fujibee#937 at all, and nothing says so. All five gates now go through `_require_launcher_bridge`, which fails and names the count it actually reached. The point-in-time count in the first test is dropped: the gate now makes the same statement, earlier. The control test is new, and was fired before this was handed over. With the gate returning 0 on exhaustion -- the behaviour the `for ... break` form had -- it goes red at `[ "$status" -ne 0 ]`; with the gate as written it is green. It costs the waiter's full sweep on purpose: an exhausted gate is the thing being measured. The one other `{1..80}` loop in this file (the fujibee#466 mid-loop cache test) was checked and is already load-bearing -- the line after it repeats the `grep -q` bare, so exhausting the loop fails there too. Whole file after the change: 29 passed, 0 failed (1 skipped).
…derr (fujibee#974) `_sqlite_sync_apply_fail` closed fd 3 as `exec 3<&- 2>/dev/null`. With no command word, bash does not scope that redirection to anything — it applies it to the shell, permanently. Every `>&2` after the first apply failure went to /dev/null. It sits on the failure path, so what it silences is the output a failing apply is about to produce. The first thing lost is the message fujibee#911 added this morning, naming which check returned 13 — the diagnostic that made today's work possible, disabled a few hundred lines away in the same file. `{ exec 3<&-; } 2>/dev/null` scopes it to the block. Two tests, because either alone passes while the other's failure ships: one lifts the driver's own function body out of the file by line range and calls it (a copy of the shape would keep passing while the driver regressed), one greps the file for a bare `exec` carrying a redirection. Both go red on the old form. Reported by @joelmitz, who also identified fujibee#911 as the first casualty.
…oml instead of replacing the link (fujibee#747) (fujibee#995) * fix(install): write Codex writable_roots through a symlinked config.toml instead of replacing the link (fujibee#747) configure_codex_sandbox() edited ~/.codex/config.toml with `awk ... > tmp && mv tmp config`. `mv` replaces a symlink with the plain temp file, so on a config.toml managed as a symlink (stow/chezmoi/manual dotfiles) the install detached the link and wrote agmsg's paths to a fresh file at the link path — the dotfiles target kept the old contents and Codex, still reading through the (now-gone) link's original location, lost the edit. The install reported success, so nothing flagged it. Deterministic whenever writable_roots or [sandbox_workspace_write] already exists and agmsg's paths do not; the no-section branch already appended with `>>`, which follows the link, so it was unaffected. Both mv-based branches now write through write_through_symlink(), which cats the temp file over the destination — a redirect follows the link and updates its target, preserving the link. Not atomic, which is fine for a few-KB config written once at install time; the third branch is left as-is. Tests reproduce the reporter's shape (a symlinked config.toml into a dotfiles dir) for both mv branches and assert the link survives and the edit lands on the target; reverting the helper to `mv` fails both. * fix(install): keep the atomic mv for a regular config.toml; write through only a symlinked one (fujibee#747 review) The first pass routed BOTH branches through an unconditional cat-redirect, which also made the common non-symlink path non-atomic: an interrupted install, a write error, or a full disk could leave an empty or partial ~/.codex/config.toml where mv had left all-old-or-all-new (co1's blocking review). Match the fix's scope to the defect's: move_into_place() now branches on the destination. A symlinked dest is written through (the fujibee#747 case); a regular dest keeps the atomic mv. The symlink arm stays non-atomic — there is no atomic write-through-a-link with plain POSIX tools — but that exposure is now confined to symlink users, whose target is typically a version-controlled dotfile, rather than imposed on every install. (Chosen over resolving the link and mv-ing onto the real path: that would need a portable readlink -f the tree deliberately avoids, and same-dir temp placement to stay atomic across filesystems — more surface than this bug warrants.) Adds the reverse control co1 asked for: a regular config.toml must come out with a NEW inode (mv renames; a truncate-in-place cat would keep it), so the atomic mv cannot be dropped again unseen. Reverting either arm now fails its own test.
…ith no --cmd (fujibee#659) * fix(install): refuse to guess between multiple installs on --update with no --cmd (fujibee#599) install.sh --update with no --cmd stopped at the first .agmsg-marked directory a glob yielded. A glob expands in collation order, not installation order, and nothing records which install came first, so on a machine with a sibling install (agmsg-dfc beside agmsg, where 0x2D sorts before 0x2F) the sibling was updated and the intended install was left untouched, while the command reported success. Update mode now enumerates every marked directory. Zero or one candidate behaves exactly as before; two or more fail closed: the candidates are listed and --cmd <name> is asked for. No name-based exclusion of backup-shaped directories: --cmd has no reserved-name validation, so any pattern that would catch a real backup can match a legitimately chosen name; a leftover backup that still carries the marker is just one more candidate, and ambiguity is what this refuses to guess through. --cmd <name> is unchanged. * test(install): the two-install bare --update case is a refusal now, and its assertions can fail fujibee#553's "bare --update does NOT force-steal a Codex shim owned by a different install" pinned, on its own words, the pre-fujibee#659 base: two installs present, bare --update proceeds, and the shim must survive. With fujibee#599 fixed a bare --update over two installs refuses before it touches anything, so the case now asserts the refusal, the message that names both installs, and the shim's owner line unchanged across it -- the property fujibee#553 protects, in its strongest form. The single-install case, where a bare --update does proceed, is unchanged and still covered by the neighbouring fujibee#553 test. The eight non-last [[ =~ ]] assertions this branch added could not fail on bash 3.2 (the enforceable-assertions check counted them); they are plain greps and refute now, and the baseline moves down to 636 as the checker asks.
…a resumed seat is unidentified (fujibee#982) (fujibee#993) * fix(session-start): stand down instead of an unfiltered watcher when a resumed seat is unidentified (fujibee#982) On a resumed session with no role-session record, the fallback emitted the generic watch.sh directive (no 4th <agent> arg). On a project with several registered seats that watcher subscribes to every pair and, as it delivers, stamps read_at and advances each pair's read cursor to the tip -- so it consumes other seats' unread mail, which those seats then never receive. Fail-open is the wrong default here. Fail closed instead: - Narrowing: when no role-session record matches but an actas <team>__<agent>.session lock is owned by this session's bare sid, re-seat to that pair and emit the role-filtered directive. The record the branch wanted and the lock that exists carry the same fact; an ambiguous 2+ match is left unidentified rather than guessed. - When the seat still cannot be established: emit a generic watcher only for a single-pair project (nobody else's mail to take); with more than one pair, emit no watcher and say why, naming `/agmsg actas <name>`. A silent no-watcher is indistinguishable from "no messages arriving", so the stand-down is explicit and points at the recovery. This is a delivery miss, not a loss: messages remain in the store and history.sh returns them. Tests (tests/test_resume_seat_guard.bats) run the directive as emitted and observe which pairs' read cursors move -- a string check on the directive text would stay green if the wrong watcher were emitted or watch.sh ignored its 4th arg. A fail-open regression advances a bystander seat's cursor (red); a broken narrowing fails to deliver the seat's own mail (red). * fix(session-start): state the resumed seat's basis honestly — recorded vs inferred from the actas lock (fujibee#982) The role-filtered directive is prose the next session reads and acts on: it launches the watcher the text describes. The narrowing path (fujibee#982) reused the record path's wording — "this session was recorded as that role's seat" — but it has no record; it inferred the seat from an actas lock this sid still owns. Telling the reader a thing was recorded when it was inferred hands them a claim they cannot check, and this whole fix exists because a directive was trusted and executed as-is. Split the sentence on how the seat was established: - record path: "was recorded as that role's seat" (unchanged). - narrowing path: names the actas lock as the basis and why it stands in for the record, so the reader can weigh "what if that lock is stale?". Tests assert both directions of the distinction, so an inferred seat can never again read as a recorded one.
…ute, not a no-op ! (fujibee#715 step 1) (fujibee#997) `! _is_unread_for_alice "ctrl:despawn"` reported ok even when the row was still unread: a bare `! cmd` is exempt from errexit on every bash (fujibee#670), so the assertion was written but watched nothing. Switching to `refute` makes it fail when the ctrl:despawn row lingers unread for alice. Verified the enforced form actually fails: with the condition forced true (the row always unread), `refute` turns the test red while the old `!` form stays green — the no-op made visible. The enforceable-assertions baseline drops 638 to 637 (this was its single remaining `!` exception). This does NOT fix the load-dependent flake fujibee#715 was filed for (the row sometimes not yet read right after despawn returns, under load). It makes that flake observable for the first time: enforced, the assertion can now go red, whereas before it never could. On this machine a reproduction harness (isolated store, the despawn scenario under eight concurrent writers) saw 0 flakes in 25 iterations, and bats-core 1.13 cannot run under the bash 3.2.57 the report measured on — so the red is not reproduced here, only made possible. If the flake is real, post-merge CI is where it will surface. fujibee#715 stays open for that fix.
…e#595) (fujibee#797) * fix(tests): widen _wait_pidfile's window and name what it saw (fujibee#595) * test(watch): keep every distinct pidfile observation, not just the last one * test(watch): name the four states a pidfile read can be in, and print what the successor is doing * test(watch): let the read decide readability, not a test that predicts it
…ine-detection # Conflicts: # scripts/drivers/storage/sqlite-sync.sh
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
3 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.
upstream fujibee#1006 (fujibee#1006) is open but not yet merged.
Merging into joelmitz/agmsg main now as a local patch, per rules/agents/agmsg-upgrade.md's pattern for
upstream-file-modifying patches (same treatment as fujibee#969/fujibee#973/fujibee#974/fujibee#378).
Adds a shared systemd user-unit probe (LoadState/ActiveState/SubState/MainPID) used by both
remote.sh statusandremote.sh sync start, so a supervised engine that never writes a pidfileis no longer invisible to sync start (which previously spawned a duplicate).
Commits: 3dcd54b, a85c740. grok (babelbiblenet-v2 project reviewer) PASSed a85c740 on 2026-08-28.
35/35 bats pass in tests/test_remote_status_liveness.bats after merging current main, including the
4 new fujibee#894 cases.
When fujibee#1006 merges upstream, sync-upstream.yml will need manual conflict resolution here
(same as the other upstream-file-modifying local patches already tracked in agmsg-upgrade.md).