Tagger.add()/remove() can break the outcome invariant that success()/fail() maintain

Bug Description

Tagger keeps the session outcome inside the same tag dict that add() writes to, but only success() and fail() maintain the mutual exclusion. add() and remove() write the dict directly, so the two public paths into the reserved lk. namespace disagree about the outcome.

success() and fail() both start by popping the opposite outcome tag (observability.py:58, :69), and outcome then reads that dict back (:102-108). A tag written through add() skips that step:

>>> t = Tagger()
>>> t.fail(reason="booking failed")
>>> t.add("lk.success")
>>> sorted(t.tags)
['lk.fail', 'lk.success']
>>> t.outcome, t.outcome_reason
('success', 'booking failed')

outcome checks lk.success first, so the session is reported as a success while session.tags still carries lk.fail (telemetry/traces.py:1302) and the reason field still holds the failure text (traces.py:1363-1365). Because the severity is derived from outcome (:1372-1373), the job is also reported at SeverityNumber.UNSPECIFIED instead of ERROR, so a failed job is not flagged as an error in LiveKit Cloud.

remove() has the mirror problem: it clears the tag but not the reason, so success("done") followed by remove("lk.success") leaves outcome is None with outcome_reason == "done".

This is reachable rather than theoretical because add() is already an established writer into the reserved namespace: the framework itself calls ctx.tagger.add(f"lk.amd:{category}", ...) at voice/amd/detector.py:552, and _evaluation writes lk.judge.* through the same dict (:120-131). Nothing in add()'s docstring says lk.* is off limits, so application code following that pattern can hit it.

Expected Behavior

Either outcome tag is present at most once, and outcome_reason is only set while an outcome tag is. outcome should not be able to disagree with tags.

Reproduction Steps

No LiveKit Cloud account or running session is needed — Tagger is standalone:

import importlib.util, pathlib, sys

p = pathlib.Path("livekit/agents/observability.py")  # any checkout
spec = importlib.util.spec_from_file_location("obs", p)
m = importlib.util.module_from_spec(spec)
sys.modules["obs"] = m
spec.loader.exec_module(m)

t = m.Tagger()
t.fail(reason="booking failed")
t.add("lk.success")
print(sorted(t.tags), t.outcome, t.outcome_reason)
# ['lk.fail', 'lk.success'] success booking failed

There is no test covering the invariant — tests/test_recording.py only uses a mocked tagger.

Environment Details

Operating System: macOS
Models Used: n/a (observability only, no model involved)
Package Versions: livekit-agents at 57b3227a7 (main)

Proposed Solution

Two consistent options, and which one you prefer is your call:

Keep the dict, enforce the invariant in the mutators. In add(), route the reserved keys through success() / fail() (or refuse them), and in remove(), clear _outcome_reason when the removed tag was the outcome tag. Smallest diff, no change to the read path.
Store the outcome in its own field. _outcome: str | None plus _tags for custom tags only, and derive the lk.success / lk.fail tag when building the report. This removes the possibility entirely, at the cost of touching outcome, tags, _evaluation and the report builder.

I lean towards 1, since traces.py and the Cloud payload both read the tags, and 2 changes the shape of what gets uploaded.

Additional Context

voice/amd/detector.py:552 and _evaluation both write lk.* through add()-style paths, so option 1 also gives those a single place that owns the reserved keys.

Hi, I'm Himanshu, new to contributing here. I see #7490 and #7491 already address this. Could I help with a small invariant-test follow-up covering add/remove mixed with success/fail and stale outcome_reason? Please let me know which existing approach to build on and whether any cases remain uncovered; I'm not claiming the implementation.

Thanks for offering, Himanshu. Both #7490 and #7491 implement option 1 from this issue, and both cover the two mixed add()/remove() cases plus the stale-reason case. #7491 routes success()/fail() through add() and has the broader test set; #7490 is the smaller diff. I do not see an invariant case either one misses, so a third PR would mostly duplicate them rather than add coverage.

The choice between the two approaches is a maintainer call. The useful next step is to read the tests in either PR and comment if you spot a case they miss — for example an outcome tag written with metadata, or a remove after a replacement. If a gap is still there after one of them merges, a small test-only follow-up would be welcome then.

@feiiiiii5

Thanks, @feiiiiii5. I did what you suggested and ran both PRs against the cases you named, plus a few around them. Main is c2b2f5c; observability.py is unchanged on today's main. #7490 is at b35a3aa and #7491 at 6ca0c95.

case	main	#7490	#7491
A `fail(r1)`; `add("lk.success")`	success / r1, tags fail+success	success / None	success / None
B `success(r1)`; `add("lk.fail")`	success / r1, tags fail+success	fail / None	fail / None
C `success(r1)`; `remove("lk.success")`	None / r1	None / None	None / None
D `add("lk.fail", metadata=…)`	fail, metadata kept	same	same
E D then `success(r2)`	success / r2	same	same
F `fail(r1)`; `success(r2)`; `remove("lk.fail")`	success / r2	same	same
G `success(r1)`; `fail(r2)`; `remove("lk.success")`	fail / r2	same	same
H `fail(r1)`; `add("lk.fail", metadata=…)`	fail / r1	fail / None	fail / None

Cells are outcome / outcome_reason.

What the run shows:

Both PRs fix A, B and C, which are the cases from the issue. Each PR's own tests fail on main and pass on its own code:
- fix(observability): keep Tagger outcome consistent when using add()/remove() #7490: 2 of 3 fail on main, 3/3 pass.
- fix: keep the outcome invariant for tags written through add() #7491: 3 of 7 fail on main, 7/7 pass.
The two cases you suggested behave correctly in both PRs:
- an outcome tag written with metadata (D, E);
- a remove after a replacement (F, G).
Neither PR has a test for an outcome tag with metadata, though. fix: keep the outcome invariant for tags written through add() #7491's metadata test uses a custom tag.
In H, both PRs behave the same and differ from main. Re-adding the current outcome tag, for example to attach metadata after fail(reason=…), drops the reason. That doesn't break the invariant, since the tag is still set, but it is a behaviour change neither PR's tests mention.

So I agree there is no case that separates the two PRs. Once one merges, a small test-only follow-up for D/E and for H, with whatever H should do, seems useful, as you said. I'm not opening a PR now.

Not covered: what LiveKit Cloud receives (telemetry/traces.py). This only checks Tagger state.

Reproduce

git clone https://github.com/livekit/agents && cd agents
for n in 7490 7491; do git fetch -q origin pull/$n/head:pr/$n; git show pr/$n:livekit-agents/livekit/agents/observability.py > /tmp/obs-$n.py; done
python compare_7486.py livekit-agents/livekit/agents/observability.py main
python compare_7486.py /tmp/obs-7490.py "#7490"
python compare_7486.py /tmp/obs-7491.py "#7491"

Scripts: compare_7486.py, demo-7486.sh

Thanks for the careful comparison and for stating the coverage limits. H is a useful behavior change to flag; it is also raised in #7491's review. The invariant in this issue does not settle whether re-adding the current outcome should preserve its reason. A maintainer decision on H would give a focused follow-up test a clear expected result.

Navigation Menu

Repository navigation

Tagger.add()/remove() can break the outcome invariant that success()/fail() maintain #7486

Bug Description

Expected Behavior

Reproduction Steps

Environment Details

Proposed Solution

Additional Context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Tagger.add()/remove() can break the outcome invariant that success()/fail() maintain #7486

Description

Bug Description

Expected Behavior

Reproduction Steps

Environment Details

Proposed Solution

Additional Context

Activity

himanshumehta10 commented on Oct 24, 2026

feiiiiii5 commented on Oct 24, 2026

himanshumehta commented on Nov 5, 2026

feiiiiii5 commented on Nov 5, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions