fix(cli): stop set-add-policy erasing the agent profile - #6576
Open
cyberzero000 wants to merge 2 commits into
Open
fix(cli): stop set-add-policy erasing the agent profile#6576cyberzero000 wants to merge 2 commits into
cyberzero000 wants to merge 2 commits into
Conversation
`cmd_set_add_policy` published a kind:10100 whose content was only
`{"channel_add_policy": ...}`. The kind is replaceable, and the relay
projects just that one field into a column
(`crates/buzz-relay/src/handlers/side_effects.rs`) — `channel_ids`, `name`,
`display_name` and `respond_to` live solely in the event body clients read.
One policy change wiped the rest of the profile, which took the agent out of
every channel's @mention picker until an operator republished by hand.
Read the current profile, merge the field, republish. Three things the merge
has to get right, each covered by a test:
- A lookup that fails, or returns a body that is not a JSON object, is an
error rather than an empty profile. Treating it as absent would republish
a single-field profile and cause the exact wipe this fixes.
- The write out-bids the stored copy's `created_at` instead of tying it.
`Db::replace_addressable_event` breaks a same-second tie by lowest event
id, so publishing at `now` is a coin flip against the ACP harness
republishing the same event concurrently.
- The lead is bounded against the relay's ±900s tolerance, not against a
tight local-clock assumption, so ordinary skew between the harness host
and the operator's does not block a policy change.
Read-modify-write on a replaceable event has no compare-and-set, so this
re-reads after publishing and redoes the merge when someone else's copy is
stored, and reports a write conflict (exit 5) rather than success when it
cannot confirm the write landed. The window is narrowed, not closed: a peer
publishing between our read and our write still loses its change.
Signed-off-by: cyberzero000 <user1@cyberzerosystems.com>
The post-publish confirmation compared the stored head's id to ours and
treated any mismatch as a conflict. That read is not pinned to the
primary: kind:10100 is global, so the filter carries neither a channel
pin nor an `until`, which makes it `RoutePredicate::Bounded` and lets
the relay serve it from a read replica whenever
`BUZZ_REPLICA_READ_MAX_AGE_MS` is set. The budget bounds how recently
the replica proved its replay position, not whether a write from a
moment ago is visible, so a read issued straight after the publish can
return the pre-write event. The loop then republished a byte-identical
event, drew `duplicate:`, and after three attempts returned exit 5 with
`retryable: true` for a policy change that was already stored.
Classify the read instead of trusting id equality. The relay's own
replace rule separates lag from a real loss: a stored copy that loses
the `created_at`/lowest-id comparison to the event we just published
cannot have replaced it, so the read is behind. Three outcomes:
- Landed — our event is the stored head. Unchanged.
- Replaced — a copy that beats ours is stored. Re-merge onto it, as
before.
- Unconfirmed — the read cannot see our write, or it failed. The relay
accepted the event, so this is a success we could not confirm: exit 0
with the documented `{event_id, accepted, message}` plus an additive
`warning`, not a conflict.
A failed confirmation read is now `Unconfirmed` rather than propagated.
The mutation has already happened by then, so borrowing the read's error
reported a stored policy change as a network failure.
Alongside that, two smaller classification fixes:
- `fetch_own_agent_profile` mapped every read error to `CliError::Other`,
turning a retryable 503 into exit 4 / `retryable: false` and an expired
`BUZZ_AUTH_TAG` into exit 4 instead of 3. Preserve the variant and add
context where the variant has room for it.
- `parse_stored_profile` defaulted a missing `content` to `""`, which
took the empty-body path and republished a single-field profile — the
wipe this command's merge exists to prevent — and defaulted a missing
`created_at` to 0, dropping the write back to a plain `now` stamp. One
guard now refuses a result row it cannot fully read; only a genuinely
absent event yields an empty body.
Comment corrections, no behavior change: there is no sleep between
attempts, `ProfileChannelUpdater` exists nowhere in this repo (this
command is the only in-repo kind:10100 publisher, so the racing peer is
an out-of-repo one), the 600s lead budget is measured against a
different clock than the relay's 900s window, and the refusal for a
non-object stored body now names a recovery path that exists.
Signed-off-by: cyberzero000 <user1@cyberzerosystems.com>
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
1 participant
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.
buzz channels set-add-policypublished a kind:10100 whose content was only{"channel_add_policy": ...}. The kind is replaceable, and the relay projectsjust that one field into a column
(
crates/buzz-relay/src/handlers/side_effects.rs) —channel_ids,name,display_nameandrespond_tolive solely in the event body clients read.So one policy change wiped the rest of the profile. Desktop's
@mentionpickerreads
channel_idsto decide whether an agent is invocable in a channel, so theagent disappeared from every channel's picker until an operator republished the
profile by hand.
Now it reads the current profile, merges the field, and republishes.
Three decisions the merge has to get right
Each is a pure function with tests, because getting any of them wrong
reintroduces the wipe.
A failed or unreadable lookup is an error, not an empty profile.
parse_stored_profilereturns an empty body only when the identity genuinelyhas no profile. A body that is not a JSON object is refused rather than
replaced — treating it as absent would republish a single-field profile and
cause the exact wipe this fixes.
The write out-bids the stored copy's
created_atrather than tying it.Db::replace_addressable_eventbreaks a same-second replaceable tie by lowestevent id, so publishing at
nowis a coin flip against the ACP harnessrepublishing the same event concurrently, and a deterministic loss against a
copy stamped later by a skewed peer. Retrying at
nowcannot win either.The lead is bounded against the relay's ±900s tolerance, not a tight
local-clock assumption. Stamping
stored + 1is only ever one second ahead ofwhichever writer produced the stored copy, so the lead measured against our own
clock is just this host's skew from that writer's. Refusing on a few seconds of
it made the command unusable wherever the harness host's clock ran ahead of the
operator's.
Confirming the write
Read-modify-write on a replaceable event has no compare-and-set.
duplicate:means the relay took the write and rolled it back — it arrives as
accepted: true, so reading onlyacceptedreports success for a discardedwrite. This re-reads after publishing, redoes the merge when someone else's copy
is now stored, and exits 5 (the CLI's write-conflict code) rather than reporting
success for a write it could not confirm landed.
The window is narrowed, not closed: a peer publishing between our read and our
write still loses its change, because a replaceable write carries the whole body
and nothing records what we replaced.
Testing
cargo test -p buzz-cli— 369 passed, 0 failed. Six new tests.Mutation-checked: making
parse_stored_profiletreat a non-object body asabsent, and making the timestamp publish at
now, fails three of the six.cargo clippy -p buzz-cli --all-targets -D warningsandcargo fmt --checkclean.
Note
Split out of #5806, which found this while debugging why an agent answered
@mentions in one channel and was silent in another. It stands alone and has nodependency on the rest of that branch.