feat(list): report state records with a state filter, and name the failed state - #116

Qualify the no-row cases for previously persisted watchers

These failures produce no row only on a first-ever start. If the watcher already has a persisted record, _start_watcher removes only its new in-memory state on context, workspace, or binding failure—and does not touch memory at all on room-resolution or session-creation failure—after which sync_watchers() calls StateStore.save(), whose merged view preserves the old disk record. The new record-based list therefore reports that watcher as failed, contrary to this supposedly exhaustive table; operators could consequently misinterpret a valid failed row or expect it to disappear. Qualify all of these cases based on whether a prior record exists.

Useful? React with 👍 / 👎.

Correct, and this is the third round on this same paragraph — which is the finding rather than the wording. Rollbacks remove only what this start added, so a watcher that ran on an earlier boot keeps its disk record through merged_view and shows failed, while a first-ever start with the identical fault shows nothing.

Rather than qualify each row, the operator-facing texts now state the invariant: a watcher appears exactly when a state record exists for it — not when it is configured, not when it started. Two consequences are called out explicitly: no row means "nothing left to act on", not "the start never got far"; and a failed row may be weeks old rather than a fresh failure.

The design doc keeps a per-failure table, because an implementer does need it, with the first-start caveat written next to it. Fixed in 4d75f4f (docs/user-guide.md, CHANGELOG.md, docs/design/dynamic-watcher-design.md).

Render non-string participant entries safely

When a persisted record contains a non-string participant, load_state() accepts it because the schema check validates only that participants is a list, but this new formatter passes the elements directly to str.join; for example, participants: [1] raises TypeError and makes the entire list command crash instead of displaying the other valid watcher rows. Either validate the element type while loading state or stringify each participant before joining.

Useful? React with 👍 / 👎.

Agreed, and ranked higher than P2 on consequence: ", ".join on a non-string loses every row from every connector, not one cell — and this loader has now been corrected seven times for a value read without checking it is the type the reader assumed. Ranked lower than P2 on reachability, since nothing writes participants yet. Net: fixed at both layers rather than choosing between them.

• Loader — an element check derived from the annotations via get_args, not special-cased for participants, so a second sequence field cannot be added without its elements being checked. A test enumerates every sequence-typed field on WatcherState and asserts each rejects a wrong element type.
• Formatter — stringifies. The CLI reads rows off a socket rather than parsing the state file, so a daemon on a different version can still hand it anything; a display function must never be what takes the command down. Tested directly with participants: [1, None, "@alice"].

One thing the fix turned up: next(..., None) cannot signal "no bad element", because None is itself a value a payload can contain — participants: [None] would have passed. Sentinel is an object(), with a test for that case.

Fixed in 4d75f4f and 9767b6a.

Follow-up: half of this fix has been reverted.

Kept — the CLI formatter stringifies. The CLI reads rows off a socket rather than parsing the state file, so a daemon on a different version can hand it anything, and a display function must never be what loses every other connector's rows. Tested with participants: [1, None, "@alice"].

Reverted — the loader's derived element-type check. Re-ranked on reachability rather than consequence: nothing writes participants yet, so the check guards a field with no producer, and it is loader hardening rather than part of the read-only view this increment is scoped to. Under this repo's own severity rules that makes it real but not urgent, and building it now is a guard whose premise has not arrived.

It should land with the increment that starts writing participants (impl/runtime-lifecycle), where a test can exercise a real producer instead of a hand-written record. Reverted in acbe308.

Require an active watcher before recommending scheduling

list --all also returns paused, idle, and failed records, none of which has the processor required by JobScheduler._inject(), so merely choosing a displayed name does not establish that scheduled messages can be delivered. A paused watcher skips every fire until resumed, while idle or failed watchers take the injection-failure path; this guidance should tell the agent to require an active row (for example, by using list --active) rather than treating presence in the all-states view as sufficient.

Useful? React with 👍 / 👎.

Half agreed — the guidance was wrong, but requiring --active is the wrong correction, and my original wording was wrong in the other direction too.

Tracing _fire_job (gateway/core/scheduler.py:358-400) rather than assuming:

• A paused watcher's fire is skipped with an explicit "expected" log and retried at the job's next occurrence; a finite job's run_count is not consumed (:380-382).
• A non-paused failure additionally posts a ⚠️ notice into the room via _notify_injection_failure, so it is not the silent no-op my text claimed.
• Idle is a non-issue by design: §2.8 requires get to recreate an idle watcher transparently precisely because scheduled work has no other route back. It also cannot occur today — nothing writes dropped_at.

So refusing on anything but active would block two legitimate cases the repo owner named: an ops pause for maintenance with a reminder set for after it, and a transient failure. Both work today.

What does need saying is when the retry lands, which varies enormously — computed rather than assumed:

That last one is why the guidance now asks the agent to inform and confirm rather than refuse: a one-shot whose only fire lands inside an outage effectively never happens, and the user should get to decide. Fixed in 4d75f4f.

Separately noted for its own issue, not this PR: --every 5m --times 1 against a failed watcher posts that ⚠️ into the room every five minutes indefinitely — there is no retry cap.

Qualify notices for disk-only failed watchers

For a watcher with a persisted record whose agent is unavailable at this boot, sync_watchers() skips it without adding it to _states, while the merged record view still lists it as failed. When its scheduled job misses, _notify_injection_failure() calls notify_watcher_room(), but that method reads only the in-memory state and returns without sending when the record is disk-only. Thus this newly documented guarantee that every failed delivery posts a notice is false for a failed state users can actually select from list --all; either hydrate the record for notification or qualify this row.

Useful? React with 👍 / 👎.

Confirmed, and thank you — this is the fourth site of a rule the previous commit fixed at three others, so I swept the rest rather than patch this one.

notify_watcher_room resolves the room through get_watcher_state, which reads the in-memory map only (gateway/core/session_manager.py:265), so a blocked-agent record — precisely the one list shows as failed — resolves to nothing and the notice is dropped.

Sweep of every other reader of watcher state, neither affected:

• inject_message (:215) only reads state after establishing a processor, so the record is in memory by construction.
• the scheduler's is_paused check (gateway/core/scheduler.py:361) is safe, because sync_watchers hydrates a paused record at watcher_lifecycle.py:142-145 — before the agent-availability check at :150-159 — so a paused record is never disk-only.

Taking the "qualify" option rather than "hydrate", deliberately. Hydrating would make the notice fire, but a disk-only record can still name a room the watcher has since moved away from — the same stale-room_id class this branch just fixed for watermarks in _stop_processor and StateStore.save — and posting a scheduling alert into a room where nobody set a reminder is worse than posting none. That makes it a notification-policy decision rather than a one-line fix, and it now sits on #117 next to the uncapped-repeat question, which is the same subject: this path notifies forever for a loaded watcher and never for an unloaded one, and both read as failed.

The agent-facing row now says the notice is not guaranteed and tells the agent not to promise it. Current behaviour is pinned by TestNotifyWatcherRoomNeedsLoadedState so changing it later is a decision rather than an accident.

Fixed in 3999000.

Retry watermark capture after a failed stop

When get_last_processed_ts() raises transiently during shutdown, _stop_processor() has already removed the watcher from _processors and aborts before copying its live cursor; stop_all() then swallows that exception, and this serving set is empty, so StateStore.save() no longer makes the retry that the previous implementation made. The stale persisted watermark can therefore cause already accepted messages to be redelivered after restart. Keep watchers whose stop failed eligible for the final watermark pull, or make the capture failure non-fatal inside _stop_processor().

Useful? React with 👍 / 👎.

Confirmed, and ranked above P2 — this is a regression I introduced last round, not a pre-existing gap, and it is silent.

The failure is worse than the transient-raise case you describe. shutdown() calls stop_all() and then save_state(), so by the time the final save runs self._processors is empty — meaning serving was empty and the shutdown save polled no room at all, for any watcher. That poll is the backstop for a watcher whose own capture failed, and losing it silently redelivers messages after a restart, reopening the class this series' fix/watermark-before-unsubscribe closed deliberately.

The mistake was the predicate. "Has a processor right now" is not what makes a record's room_id trustworthy enough to read the connector's cursor for — "this process started it" is, because then we resolved that room ourselves and subscribed to it. And unlike a processor, that survives the stop. _started is now the serving set, never cleared on stop, with the reason written next to it.

Also took your second option in addition: the stop-path read is now best-effort, as the same read already is in StateStore.save. One value read in two places should not have two failure policies, and aborting there skipped the unsubscribe and the drain in order to preserve a watermark.

Three tests: the final save polls a stopped watcher; a hydrated record is never polled; a raising cursor read does not abort the stop. Fixed in 367c9d4.

Follow-up: this fix has been reverted, because the thing it restored does not exist.

Your finding was right about the mechanism — the shutdown save's serving set really was empty — but I accepted "and therefore the retry is lost" without checking whether that retry could ever succeed. It cannot. unsubscribe_room drops the connector's room entry (rocketchat/connector.py:1031) before save_state() runs, so the save's read returns None for every stopped watcher.

Measured with a connector that models the pop, rather than reasoned about:

normal shutdown                          -> persisted: 'T-100'
stop-time capture raises (save read fine) -> persisted: ''

The stop-time capture is what persists the watermark, and always was — that is what fix/watermark-before-unsubscribe established. There was no second chance to remove.

So _started, the serving parameter, and the _hydrated_state write that made them necessary are all gone. The whole chain was downstream of fixing a pre-existing defect inside an increment the plan defines as a read-only view over persisted records — that defect is now #118 instead.

Reverted in acbe308.

Avoid claiming every absent watcher never started

A missing row proves only that no state record survived, not that startup never began: _start_watcher() can create a session and progress through context, workspace, or binding work before rolling its newly written record back. The corrected rule in docs/user-guide.md:674-685 explicitly distinguishes those cases, so this scheduling guidance can make the agent give users a false diagnosis; describe the watcher as having no surviving record and treat it as failed without asserting how far startup got.

Useful? React with 👍 / 👎.

Agreed — the row overclaimed, and docs/user-guide.md had already been corrected on exactly this point, so the two were inconsistent within the same PR.

It now reads: no state record survived; treat it as failed — but do not tell the user it never started: a start can create a session and get well into setup before rolling its record back. Fixed in 367c9d4.

Reject non-list state-filter payloads

When a hand-written control-socket client sends a JSON object such as {"states": {"idle": false}}, this loop iterates the object's keys and accepts the request as an idle filter even though the wire contract requires a list. The caller therefore receives a successful answer to a malformed and potentially different query, contradicting the function's stated fail-loud behavior; validate that names is a list before iterating it.

Useful? React with 👍 / 👎.

Correct, and the docstring above it claimed fail-loud, which made it worse than a silent gap — the function advertised the behaviour it did not have.

parse_state_filter now requires a list. Worth noting the same check rejects a bare string, which was the more likely accident: "idle" iterates to its characters, so it would have failed as five unknown names — the right outcome for the wrong reason — and "" would have read as an empty selection. Both cases are tested. Fixed in 367c9d4.

Mark the watcher started only after subscription succeeds

When startup fails after this line, _started permanently contains a watcher that never successfully subscribed. In particular, subscription rollback deliberately retains ws, so the subsequent sync_watchers() save treats the failed watcher as serving and polls its room cursor; if the connector left partial room state or cleanup failed, that cursor can represent messages that were never routed to this processor, and restoring it on the next boot silently skips them. Add the name only after subscription and dispatcher registration succeed, while continuing to retain it across later normal stops.

Useful? React with 👍 / 👎.

Follow-up: moot — _started no longer exists.

Your timing analysis was correct, and I fixed it as reported. It has since been reverted along with the mechanism it belonged to: _started was introduced to restore a shutdown watermark poll that measurement showed is inert (see the thread on watcher_lifecycle.py:475), so the field, its early-marking bug, and the nineteen fixture edits it forced are all gone.

Worth recording that this finding is what made the pattern visible: three consecutive rounds landed findings in the previous round's fix, which is the signal that the approach rather than the code was producing them. Reverted in acbe308.

Remove the false never-started diagnosis

When context injection, attachment workspace setup, or session binding fails, startup may have already created a session and performed substantial work before rolling back the record, so a missing row does not establish that the watcher never started. Fresh evidence in the current tree is the immediately following troubleshooting step, which explicitly describes these rollback cases and says the opposite; leaving both statements adjacent can send operators to the wrong diagnosis.

Useful? React with 👍 / 👎.

Restore hydration before claiming verbs preserve disk records

When a watcher is skipped because its agent is unavailable, its persisted record is visible through merged_view() but absent from WatcherLifecycle._states; pause_watcher() therefore fabricates a blank record and resume_watcher() starts with state=None, after which the in-memory record wins during save. Thus the behavior described here as fixed is still present: following this guidance can silently discard the session ID and watermark, causing lost continuity and message redelivery. Either hydrate the persisted record before these verbs act or remove this release-note guarantee.

Useful? React with 👍 / 👎.

Correct, and it is the clearest kind of finding: the entry described behaviour this branch had for four commits and then reverted, on the one document a released user reads.

The revert is deliberate — the increment is a read-only view over persisted records, and making the verbs agree with list requires writes that do not belong in it. The underlying defect is real, pre-existing, and now tracked as #118, where it is also the one of three worth fixing.

The entry now says that instead: list can show a record the verbs cannot act on, this change makes it visible rather than introducing it, and #118 carries it.

Worth recording the class, since it is not one I had a check for: a revert has to sweep the prose it wrote, and that prose is not in the diff being reverted. Nothing points from _hydrated_state back to the paragraph describing it. Fixed in b8630aa.