Skip to content

fix(scheduler): drop the activity-hint query nothing has read since March - #1220

Open
lilyshen0722 wants to merge 1 commit into
mainfrom
fix/activity-hint-dead-recent-messages-query
Open

fix(scheduler): drop the activity-hint query nothing has read since March#1220
lilyshen0722 wants to merge 1 commit into
mainfrom
fix/activity-hint-dead-recent-messages-query

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

What

Message.findActivityHint issued two queries per pod per heartbeat tick. The second one — the three most recent messages, LEFT JOINed to users for a username — has had no reader anywhere in the repo since 2026-03-07.

Why it survived five months

  • 089d0058 added it: the heartbeat's Mongo-backed query returned nothing, so "agents never saw pod chat activity". The rows went into activityHint.recentMessages for the model to read.
  • 8608060d removed recentMessages 23 minutes later — agents fetch chat themselves via commonly_get_messages — but removed it from the consumer only. buildHeartbeatActivityHint reads .count and .lastAt; the producer kept running the join.

A query with no reader cannot break, which is why nothing ever flagged it.

How it was found

Sideways. @sprint-review audited this query for a missing column — thread_root_id, which the two real read paths (findById, findByPodId) carry and this one does not. The right answer was that adding it would have been adding a field to dead code.

Worth generalizing: when a projection looks incomplete, check that it has a reader before deciding what it owes them.

Tests

Five guards where there were none. One note on the control, because it caught a weak assertion:

The join/LIMIT 3 assertion matches across every recorded call rather than calls[0]. The dead query ran inside a Promise.all beside the aggregate, so the aggregate was still first — the calls[0] form stayed green against the exact code the test exists to reject, and was the one assertion of five the negative control did not redden.

Corrected: all five fail against the pre-fix model, all five pass after. __tests__/unit/models/pg/ 14/14, tsc clean.

🤖 Generated with Claude Code

…arch

`findActivityHint` ran two queries per pod per heartbeat tick. The second —
three most recent messages, LEFT JOINed to `users` for a username — has had
no reader anywhere in the repo since 2026-03-07.

The history is the whole story. 089d005 added it because the heartbeat's
Mongo-backed query returned nothing and "agents never saw pod chat activity".
8608060 removed `recentMessages` from the hint 23 minutes later, on the
grounds that agents fetch chat themselves via commonly_get_messages — but
removed it from the CONSUMER only. The producer kept joining, five months,
for a value discarded at the call site.

Found sideways: @sprint-review (57711) audited this query for a MISSING
column (`thread_root_id`, absent here where the two real read paths carry
it). The right answer was that adding it would have been adding a field to
dead code. A projection that looks incomplete is worth checking for a reader
before deciding what it owes one.

Five guards where there were none — nothing failed while the dead join was
there, which is precisely why it survived. The join/LIMIT assertion matches
across every recorded call rather than `calls[0]`: the dead query ran inside
a Promise.all beside the aggregate, so the aggregate was still first, and the
calls[0] form stayed green against the exact code the test rejects. It was
the one assertion of five the negative control did not redden. Corrected, all
five now fail against the pre-fix model and pass after; pg suite 14/14.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at head:96ecab64. I verified the deletion claim independently rather than accepting it, because "nothing reads this" is a reachability claim and a broken search finds no callers just as convincingly as a real absence.

The barrier, not just the absence. findActivityHint has exactly two references in the repo outside tests: its definition at Message.ts:442 and the single call at schedulerService.ts:890. That call site annotates the awaited result as { count: number; lastAt?: string | Date }recentMessages is not in its own type — and it builds the returned hint as an explicit object literal, not a spread. That last part is what actually closes it: a ...pgMsgHint there would have shipped the field into the heartbeat payload and given it a reader outside the repo (the model), where no grep would find it. It doesn't.

Commits check out exactly. 089d0058 2026-03-07T17:25:30-08:00, 8608060d 2026-03-07T17:48:43-08:00 — 23m13s apart, so "23 minutes later" is right rather than rounded.

Tests pass — 5/5 on the branch.

One observation on the negative control, which strengthens your case rather than weakening it: issues exactly one query independently reddens against the reverted code, so the suite was already failing closed even with the weak calls[0] assertion. What you fixed was a redundancy gap, not an open hole — the reverted code could not have landed green. Worth stating precisely because "4 of 5 reddened and the important one didn't" reads as a near-miss on correctness, and it wasn't one.

Not verified

  • I did not re-run your negative control myself; I reasoned about which assertions reject the reverted code from reading them, and only ran the suite in its fixed form.
  • I did not check consumers outside this repo. The explicit-object-literal argument above is what rules that out, not a search.
  • Trivial and non-blocking: the surviving SQL's continuation lines are now indented one level deeper than the opening SELECT, so the logged query text is ragged. Cosmetic only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant