[bpamiri]

Summary

Fixes four channels/SSE findings from the internal framework review: SSE blank-line framing loss, a database-channel poll cursor that never advanced past filtered events, empty-channel map leakage in the in-memory Channel engine, and an unbounded retention DELETE running inside DatabaseAdapter.publish(). A review follow-up commit replaces a Lucee-only DirectoryCreate(path, true) call in the new spec with the engine-agnostic java.io.File.mkdirs() pattern so the spec compiles on Adobe CF.

Findings addressed

• C12 — blank lines silently dropped from multi-line SSE data @ vendor/wheels/controller/sse.cfc:194 — $formatSSEEvent now passes includeEmptyFields=true to ListToArray, so blank lines are preserved as empty data: fields per the SSE spec instead of collapsing adjacent lines.
• C15 — $subscribeDatabase poll cursor stuck behind filtered events @ vendor/wheels/controller/channels.cfc:257-259 — the cursor advances per-row before the event-type filter continue, so non-matching events newer than the last match are no longer re-fetched and re-filtered on every poll for the life of the connection.
• J9 — Channel.unsubscribe() never prunes empty channel maps @ vendor/wheels/Channel.cfc:126-141 — the last unsubscriber now removes the per-channel ConcurrentHashMap via the atomic two-arg remove(key, value); the residual subscribe/prune race (same orphan class as before, self-healing) is documented inline.
• J12 — unbounded retention DELETE inside publish() @ vendor/wheels/channel/DatabaseAdapter.cfc:26-29,150-171,213-222 — lastCleanup initializes to now (no sweep on a fresh instance's first publish); the throttled publish-path pass is bounded to cleanupBatchSize (1000) oldest expired rows via an indexed id IN-list DELETE, with a 15s catch-up interval while a backlog drains; cleanup() gains a maxRows argument and a deleted-row-count return for scheduled-task use. Documented deviation from the review recommendation: the sweep stays on the publish path (now bounded, so non-blocking) rather than introducing the framework's first runtime cfthread.

Review follow-up (blocking item)

• vendor/wheels/tests/specs/controller/channelSpec.cfc used DirectoryCreate(stubDir, true) — the createPath flag is Lucee-only (Adobe CF rejects the second argument, and a compile error in any spec crashes the whole directory-compiled core suite). Replaced with CreateObject("java", "java.io.File").init(stubDir).mkdirs(), the pattern established in reloadGlobalsSpec.cfc for exactly this issue (Packages page fails on Adobe ColdFusion: DIRECTORYCREATE called with unsupported arguments #2567); mkdirs is a no-op when the directory exists, so the DirectoryExists guard was dropped.

Findings verified already-fixed

None — all four findings reproduced against the pre-fix code on develop (staleRefs=[]; the reviewer independently confirmed via diff minus-lines that develop had the 2-arg ListToArray, the post-continue cursor assignment, no channel pruning, and lastCleanup=0).

Source

Internal multi-agent framework review 2026-06-09, wave 2, package channels-sse.

Tests

• vendor/wheels/tests/specs/controller/sseSpec.cfc — new spec asserting $formatSSEEvent emits data: line one\ndata: \ndata: line two\n\n for input containing a blank line (fails pre-fix: blank line dropped).
• vendor/wheels/tests/specs/controller/channelSpec.cfc — new MockBox-driven spec bounding $subscribeDatabase to exactly two polls (stubbed checkError false→true) and asserting pollLog[2].lastEventId == "evt-2" (pre-fix it stays "start-id"), plus zero deliveries of filtered events.
• vendor/wheels/tests/specs/controller/channelSpec.cfc / vendor/wheels/tests/specs/channel/DatabaseAdapterSpec.cfc — pruning + bounded-cleanup specs (empty-channel map removed after last unsubscribe; fresh-instance first publish performs no sweep; cleanup(maxRows=N) deletes at most N rows and returns the count).
• Local verification: Lucee 7 + SQLite green at implementation time (sseSpec 22, channelSpec 25, DatabaseAdapterSpec 10). After the follow-up fix, the channelSpec bundle was re-verified on Adobe 2023 + SQLite via the worktree single-bundle Docker recipe: 25 pass, 0 fail, 0 error — exercising the suite's first MockBox $()/$results() stub-generation path on an Adobe engine, as the reviewer requested. Full matrix deferred to CI (compat-matrix.yml).

Cross-engine notes

• The follow-up commit removes the only Adobe-incompatible construct (DirectoryCreate 2-arg form). java.io.File.mkdirs() is the repo's established engine-agnostic pattern (reloadGlobalsSpec.cfc, issue Packages page fails on Adobe ColdFusion: DIRECTORYCREATE called with unsupported arguments #2567).
• The new specs avoid the known traps: no inline closures as constructor named args, ##-escaped issue references in comments, shared-struct state where closures are involved.
• MockBox stub generation writes (and normally removes) stubs under /testbox/system/stubs, which resolves under public/ (no /testbox mapping in public/Application.cfc) — in bind-mounted dev trees this can leave an untracked public/testbox/ directory behind (observed and cleaned during local Adobe verification). Harmless and test-infra-only; flagged here per the reviewer's non-blocking observation.
• cleanupBatchSize=1000 sits exactly at Oracle's ORA-01795 IN-list limit (1000 itself is allowed); any future increase must chunk the IN-list — the surrounding try/catch would degrade to log-and-return-0 rather than fail publishes.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR fixes four real bugs in the channels/SSE stack — blank-line framing loss, a stuck DB poll cursor, empty-channel map accumulation, and an unbounded retain DELETE on the publish path. The implementations are correct and cross-engine, the specs cover happy path and edge cases in BDD style, and the follow-up commit correctly swaps the Lucee-only DirectoryCreate(path, true) call for the engine-agnostic Java mkdirs() pattern. No blocking correctness, cross-engine, or security findings. A few documentation and test-precision nits below. Comment (no changes required).

---

Correctness

Bounded cleanup returns SELECT candidate count, not actual DELETE count

vendor/wheels/channel/DatabaseAdapter.cfc, lines 184–186 (post-patch):

queryExecute(
    "DELETE FROM wheels_events WHERE createdAt < :cutoff AND id IN (:ids)",
    ...
);
return local.candidates.recordCount;

local.candidates.recordCount is the number of rows selected for deletion, not the number actually deleted by the DELETE statement. In a concurrent deployment where another server races to delete some of those same rows between the SELECT and DELETE, the returned count is higher than the actual rows removed. The immediate consequence is that $maybeCleanup may set cleanupBacklog = true and schedule 15-second catch-up passes when the backlog was actually empty. This self-corrects on the next pass (which finds fewer than cleanupBatchSize rows) — so it is not a crash or data-loss risk, and the function is already documented as "best effort." Worth calling out because the asymmetry between the bounded and unbounded paths means a future refactor could introduce a real count mismatch.

If a more accurate count is needed later, capturing the DELETE result with result: "local.deleteResult" (the same pattern used on the unbounded path) and reading local.deleteResult.recordCount would close the gap.

---

Conventions

cleanupBatchSize = 1000 sits at Oracle's ORA-01795 limit — missing inline warning

vendor/wheels/channel/DatabaseAdapter.cfc, line 29:

// Cap on rows deleted per throttled publish-path cleanup pass
variables.cleanupBatchSize = 1000;

The PR body correctly notes "1000 itself is allowed; any future increase must chunk the IN-list." That warning lives only in the PR description, which developers won't see when reading the code. Consider promoting it to the inline comment:

// Cap on rows deleted per throttled publish-path cleanup pass.
// 1000 is Oracle's ORA-01795 IN-list ceiling (inclusive) — do not raise
// without chunking the DELETE IN-list in cleanup().
variables.cleanupBatchSize = 1000;

---

Tests

toBeGTE(2) poll-count assertion could mask a cursor regression

vendor/wheels/tests/specs/controller/channelSpec.cfc, line 301:

expect(ArrayLen(pollLog)).toBeGTE(2);
// The cursor must have advanced past the filtered events
expect(pollLog[2].lastEventId).toBe("evt-2");

The test mocks exactly two poll() results (firstPoll, emptyPoll) and two checkError() results (false, true), so in practice exactly two polls occur. toBeGTE(2) would still pass if the cursor failed to advance but the loop happened to emit three or more polls before checkError fired — the second-index assertion would catch that, but only for pollLog[2]. Using toBe(2) is a tighter and cheaper guard:

expect(ArrayLen(pollLog)).toBe(2);

---

Commits

Both commits pass commitlint and carry Signed-off-by DCO trailers. One minor observation: commit 3b99eb6c5 is titled fix(sse): blank-line framing, DB channel cursor advance, empty-channel pruning but its diff also includes the bounded-retention cleanup work in DatabaseAdapter.cfc — the most complex change in the PR. The PR title correctly names all four items; the individual commit subject leaving out the fourth makes git log --oneline slightly misleading. Not a policy violation, just a heads-up for the author.

---

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and non-sycophantic. Each finding was verified against the diff. No false positives. One minor inaccuracy in the reasoning behind the toBeGTE(2) finding, but the recommendation itself is sound. I align with A's COMMENTED verdict — no changes required.

Sycophancy

None detected. A's COMMENTED verdict (no changes required) is earned: all four findings are genuine but non-blocking, and A correctly distinguished severity rather than reflexively approving.

False positives

None detected.

• SELECT count vs DELETE count (DatabaseAdapter.cfc, bounded path): verified. The bounded cleanup() path returns local.candidates.recordCount (the SELECT count) without capturing result: on the DELETE statement, while the unbounded path uses result: "local.deleteResult". A's claim is accurate. The self-correcting nature and "best effort" framing make it non-blocking.
• Oracle ORA-01795 inline comment: verified. The inline comment at variables.cleanupBatchSize = 1000 reads only // Cap on rows deleted per throttled publish-path cleanup pass with no Oracle limit note. A's claim is accurate.
• toBeGTE(2) assertion: verified. The test code at channelSpec.cfc line 301 uses toBeGTE(2). A's recommendation to tighten to toBe(2) is correct advice. However, A's stated reasoning — "if the cursor failed to advance but the loop happened to emit three or more polls before checkError fired" — cannot actually happen with this mock: $results(false, true) means any call past the second returns the last value (true), so the loop always terminates after exactly two polls regardless of cursor behavior. The real reason toBe(2) is better is that it precisely documents expected behavior. Minor reasoning imprecision; the recommendation stands.
• Commit subject asymmetry: cosmetic, not a policy violation. A correctly identifies it as advisory only.

Missed issues

One borderline item worth noting: the bounded cleanup() path builds the IN-list via ValueList(local.candidates.id) and binds it with list: true (comma-delimited splitting). An event ID containing a literal comma would be split into multiple tokens, causing the IN-list to include unexpected fragments. The code comment acknowledges this ("even if an application-supplied event id contains a comma") and partially mitigates it by repeating the cutoff predicate — so only expired rows are ever deleted, but some intended targets might be missed in a pass. Given that (a) the author documented awareness, (b) the mitigation prevents data loss, and (c) this is a best-effort cleanup path, A's choice not to raise it as a finding is defensible. Not blocking.

Verdict alignment

A's COMMENTED verdict is consistent with the findings. All four items are documentation nits, test-precision suggestions, or cosmetic observations — none rises to request-changes territory.

Convergence

Aligned. A's findings are accurate, the verdict is appropriate, and I have no unresolved missed-issue findings. The PR is review-clean for this SHA.