[Bug][Backend][API] CodeExecutionResultConsumer uses autoAck:true — a student's execution result is silently lost if the API dies mid-handle #573

New Issue

2026-07-14T14:58:53Z

spikerj commented

2026-07-14 14:58:53 +00:00

QA Team — filed 2026-07-14 from the README audit, at spikerj's direction (decision: file).

The bug: the API's CodeExecutionResultConsumer consumes the code.execution.responses queue with autoAck: true — RabbitMQ considers the message delivered the moment it hits the socket, before the handler has forwarded the result to SignalR. If the API instance dies (or the SignalR forward throws) between delivery and completion, the result is gone: the student's code ran, the worker did the work, and the browser waits forever with no error.

This is at-most-once delivery on a user-visible result path. Every other consumer in the codebase uses autoAck: false with a manual ack after successful handling (e.g. KeycloakEvents, the notification consumers) — this one is the outlier.

Fix shape: autoAck: false + manual BasicAck after the SignalR forward succeeds, with the standard retry/DLQ wiring (RabbitMQRetryHelper, Rabbit:DeadLetterQueue) the other consumers already use. Consider a short TTL on the queue so a stale result doesn't get replayed to a user who has moved on.

Related: #570 (no coderunner execution traces exist in Jaeger — so today a lost result would also be invisible to tracing).

**QA Team** — filed 2026-07-14 from the README audit, at spikerj's direction (decision: file). **The bug:** the API's `CodeExecutionResultConsumer` consumes the `code.execution.responses` queue with **`autoAck: true`** — RabbitMQ considers the message delivered the moment it hits the socket, before the handler has forwarded the result to SignalR. If the API instance dies (or the SignalR forward throws) between delivery and completion, the result is **gone**: the student's code ran, the worker did the work, and the browser waits forever with no error. This is at-most-once delivery on a user-visible result path. Every other consumer in the codebase uses `autoAck: false` with a manual ack after successful handling (e.g. KeycloakEvents, the notification consumers) — this one is the outlier. **Fix shape:** `autoAck: false` + manual `BasicAck` after the SignalR forward succeeds, with the standard retry/DLQ wiring (`RabbitMQRetryHelper`, `Rabbit:DeadLetterQueue`) the other consumers already use. Consider a short TTL on the queue so a stale result doesn't get replayed to a user who has moved on. **Related:** #570 (no coderunner execution traces exist in Jaeger — so today a lost result would also be invisible to tracing).

spikerj commented

2026-07-14 20:02:48 +00:00

Worked in spikersoft-backend PR #282 (open, not yet merged). The premise here is inverted — flagging it because the audit that produced this ticket will produce more like it.

SpikerSoft.Business/Services/CodeExecutionResultConsumer.cs really does use autoAck: true, but it is dead code: never DI-registered (a repo-wide grep for the name returns only its own two files), never injected, and it never touches SignalR — it parks results in a ConcurrentDictionary nothing reads.

The actual consumer of code.execution.responses is SpikerSoft.Api/HostedServices/SignalRNotificationConsumerService.OnCodeExecutionResultReceived, and it already does every single thing this ticket asks for: autoAck: false, BasicAck only after the SignalR forward succeeds, retry/DLQ via RabbitMQRetryHelper + Rabbit:DeadLetterQueue (which defaults to Enabled = true), W3C trace context via ActivityHelper, poison detection, QoS prefetch. There is no at-most-once loss on the live path. No student result is being dropped today.

The dead class still had to go, though, because it is armed rather than inert: its constructor fires Task.Run(InitializeConsumerAsync), which attaches an autoAck: true consumer to code.execution.responses. Register it — and it ships an ICodeExecutionResultConsumer interface that invites exactly that — and RabbitMQ round-robins deliveries between it and the real SignalR forwarder. Half of every student's results get auto-acked into the unread dictionary and lost, and the browser waits forever. That is this ticket's failure mode, one AddSingleton away. PR #282 deletes the class and its interface.

It also adds CodeExecutionResultDeliveryTests (Testcontainers, real broker) to lock the two properties that make the failure mode impossible: exactly one consumer on the queue, and a result whose SignalR forward keeps throwing gets retried and dead-lettered with forensic headers rather than acked away. Neither test fails on master — the defect was latent, so a fail-before test isn't constructible — they're forward guards plus new coverage of the failure contract, which was previously untested.

Two notes for the next pass of the same audit:

SpikerSoft.Business/Domain/Lessons/Services/LessonRegradeClient.cs:187 is the only other autoAck: true consumer. It is correct: RabbitMQ's Direct-Reply-To pseudo-queue requires no-ack, and the code comment says so. Please don't re-file it.
The "every other consumer uses autoAck: false, this one is the outlier" observation was right — the useful follow-through is checking whether an outlier is reachable before writing up user-visible impact.

Leaving this open until #282 merges.

Worked in spikersoft-backend PR #282 (open, not yet merged). **The premise here is inverted — flagging it because the audit that produced this ticket will produce more like it.** `SpikerSoft.Business/Services/CodeExecutionResultConsumer.cs` really does use `autoAck: true`, but **it is dead code**: never DI-registered (a repo-wide grep for the name returns only its own two files), never injected, and it never touches SignalR — it parks results in a `ConcurrentDictionary` nothing reads. The actual consumer of `code.execution.responses` is `SpikerSoft.Api/HostedServices/SignalRNotificationConsumerService.OnCodeExecutionResultReceived`, and it already does every single thing this ticket asks for: `autoAck: false`, `BasicAck` only after the SignalR forward succeeds, retry/DLQ via `RabbitMQRetryHelper` + `Rabbit:DeadLetterQueue` (which defaults to `Enabled = true`), W3C trace context via `ActivityHelper`, poison detection, QoS prefetch. **There is no at-most-once loss on the live path.** No student result is being dropped today. The dead class still had to go, though, because it is armed rather than inert: its *constructor* fires `Task.Run(InitializeConsumerAsync)`, which attaches an `autoAck: true` consumer to `code.execution.responses`. Register it — and it ships an `ICodeExecutionResultConsumer` interface that invites exactly that — and RabbitMQ round-robins deliveries between it and the real SignalR forwarder. Half of every student's results get auto-acked into the unread dictionary and lost, and the browser waits forever. That is this ticket's failure mode, one `AddSingleton` away. PR #282 deletes the class and its interface. It also adds `CodeExecutionResultDeliveryTests` (Testcontainers, real broker) to lock the two properties that make the failure mode impossible: exactly one consumer on the queue, and a result whose SignalR forward keeps throwing gets retried and dead-lettered with forensic headers rather than acked away. Neither test fails on `master` — the defect was latent, so a fail-before test isn't constructible — they're forward guards plus new coverage of the failure contract, which was previously untested. Two notes for the next pass of the same audit: - `SpikerSoft.Business/Domain/Lessons/Services/LessonRegradeClient.cs:187` is the only other `autoAck: true` consumer. It is **correct**: RabbitMQ's Direct-Reply-To pseudo-queue *requires* no-ack, and the code comment says so. Please don't re-file it. - The "every other consumer uses `autoAck: false`, this one is the outlier" observation was right — the useful follow-through is checking whether an outlier is *reachable* before writing up user-visible impact. Leaving this open until #282 merges.

spikerj commented

2026-07-14 20:24:17 +00:00

Resolved in spikersoft-backend PR #282 (merged to master as 760667c). Deleted the dead CodeExecutionResultConsumer + ICodeExecutionResultConsumer, and added CodeExecutionResultDeliveryTests guarding the two properties that make this ticket's failure mode impossible: exactly one consumer on code.execution.responses, and a result whose SignalR forward keeps throwing is retried then dead-lettered rather than acked away.

Restating the finding for the record, since it inverts the ticket: the live path (SignalRNotificationConsumerService) was already correct — no student result was being lost. The dead class was a landmine, not an active bug. Closing.

Resolved in spikersoft-backend PR #282 (merged to `master` as `760667c`). Deleted the dead `CodeExecutionResultConsumer` + `ICodeExecutionResultConsumer`, and added `CodeExecutionResultDeliveryTests` guarding the two properties that make this ticket's failure mode impossible: exactly one consumer on `code.execution.responses`, and a result whose SignalR forward keeps throwing is retried then dead-lettered rather than acked away. Restating the finding for the record, since it inverts the ticket: the live path (`SignalRNotificationConsumerService`) was **already correct** — no student result was being lost. The dead class was a landmine, not an active bug. Closing.

spikerj closed this issue

2026-07-14 20:24:20 +00:00

spikerj referenced this issue

2026-07-14 21:12:06 +00:00

[Bug][Backend][Books] Chapter extraction is orphaned end-to-end — nothing ever populates Book.Chapters, and GET /books/{id}/chapters returns [] for every book, always #593

spikerj referenced this issue

2026-07-14 21:12:22 +00:00

[Bug][Backend] BookAnalysisService (ExtractChapters / AnalyzeBookWithLLM) silently ships degraded book data on GPU/model-load failure — same class as quiz-gen #553, different pipeline #557

spikerj referenced this issue

2026-07-14 21:31:22 +00:00

[Bug][Backend][Observability] #575 was fixed in ONE publisher — BookController (5 publish sites) and 3 others still ship RabbitMQ messages with no traceparent, so user-initiated flows appear in Jaeger as disconnected orphan traces #597

spikerj referenced this issue