[Bug] Offline pass with unverified server response regresses on next refresh #73

New Issue

2026-05-06T04:35:24Z

spikerj commented

2026-05-06 04:35:24 +00:00

Summary

Follow-up to spikersoft-issues#72. The fix there ensures an offline pass survives a refresh as long as it's still in the IDB outbox. There's a second path the fix doesn't cover: when the queue flushes successfully but the server returns status: "unverified" for the item (it accepted the submission but didn't / couldn't replay it through Roslyn / CPython / Node to reproduce the pass), the lesson regresses to locked on the next refresh.

Reproduction

Pass a lesson while in-browser compile is on.
Wait for ProgressSyncService.flush() to complete (refocus the tab to trigger it, or rely on the new opportunistic flush from #72).
Server responds { status: "unverified" } for the item (e.g. test-harness state was lost, server-side runtime unavailable).
Refresh the page.
Expected: the lesson stays unlocked; the user passed it locally and the server didn't reject it.
Actual: the lesson is locked again.

Why it happens

In ProgressSyncService.flush() (libraries/tools/src/services/csharp-runner/progress-sync.service.ts):

const terminalReasons = new Set([...]);
const rejectedTokens = new Map((result.errors ?? []).map((e) => [e.attemptToken, e.reason]));
for (const item of items) {
    const reason = rejectedTokens.get(item.attemptToken);
    const shouldDelete =
        !reason || // accepted (verified or unverified)  ← deletes on `unverified`
        terminalReasons.has(reason);
    if (shouldDelete && item.id !== undefined) {
        await this.idb.deletePendingProgress(item.id);
    }
}

And in LanguageRunner's lastFlushItemsSignal handler:

if (r.status === "verified") {
    verifiedNumbers.push(r.lessonNumber);
    ...
} else if (r.status === "server_grading_failed" || r.status === "state_expired") {
    rejected.push(r.lessonNumber);
}
// `unverified` is silently dropped

Net effect for an unverified outcome:

The IDB row is deleted (so getPendingPassedLessonNumbers() won't return it on next load).
The local completedLessonsSignal retains the lesson for the rest of the session (from the local optimistic effect at lesson-completion time).
Server's /Lessons/progress doesn't include the lesson (it's unverified).
→ Refresh: nothing seeds the lesson into the completed set; it's locked.

Acceptance criteria

Decide on the right policy and implement it:

Option A (lenient): treat unverified like verified for the local UX — push it into completedLessonsSignal from the flush handler. The lesson stays unlocked; the user just doesn't earn associated skills until a future re-attempt grades cleanly.
Option B (durable): keep the IDB row when the response is unverified (only delete on verified + terminal-rejection reasons) so the next page load can re-merge it via getPendingPassedLessonNumbers() and the next flush can re-attempt verification.

Both paths fix the symptom; A is simpler, B preserves the chance to eventually earn skills. A combination (A for the UX + B for the retry pathway) is also reasonable.

Whichever path is chosen, add a regression test in progress-sync.service.spec.ts that flushes with an unverified item and asserts the chosen behavior.

Scope hints

libraries/tools/src/services/csharp-runner/progress-sync.service.ts — flush deletion policy.
libraries/tools/src/components/language-runner/language-runner.ts — lastFlushItemsSignal effect (the else if branch that splits verified vs rejected).
libraries/tools/src/services/csharp-runner/progress-sync.service.spec.ts — coverage.

## Summary Follow-up to [spikersoft-issues#72](https://git.spikersoft.com/spikerj/spikersoft-issues/issues/72). The fix there ensures an offline pass survives a refresh **as long as it's still in the IDB outbox**. There's a second path the fix doesn't cover: when the queue flushes successfully but the server returns `status: "unverified"` for the item (it accepted the submission but didn't / couldn't replay it through Roslyn / CPython / Node to reproduce the pass), the lesson regresses to locked on the next refresh. ## Reproduction 1. Pass a lesson while in-browser compile is on. 2. Wait for `ProgressSyncService.flush()` to complete (refocus the tab to trigger it, or rely on the new opportunistic flush from #72). 3. Server responds `{ status: "unverified" }` for the item (e.g. test-harness state was lost, server-side runtime unavailable). 4. Refresh the page. 5. **Expected:** the lesson stays unlocked; the user passed it locally and the server didn't reject it. 6. **Actual:** the lesson is locked again. ## Why it happens In `ProgressSyncService.flush()` (`libraries/tools/src/services/csharp-runner/progress-sync.service.ts`): ```ts const terminalReasons = new Set([...]); const rejectedTokens = new Map((result.errors ?? []).map((e) => [e.attemptToken, e.reason])); for (const item of items) { const reason = rejectedTokens.get(item.attemptToken); const shouldDelete = !reason || // accepted (verified or unverified) ← deletes on `unverified` terminalReasons.has(reason); if (shouldDelete && item.id !== undefined) { await this.idb.deletePendingProgress(item.id); } } ``` And in `LanguageRunner`'s `lastFlushItemsSignal` handler: ```ts if (r.status === "verified") { verifiedNumbers.push(r.lessonNumber); ... } else if (r.status === "server_grading_failed" || r.status === "state_expired") { rejected.push(r.lessonNumber); } // `unverified` is silently dropped ``` Net effect for an `unverified` outcome: - The IDB row is deleted (so `getPendingPassedLessonNumbers()` won't return it on next load). - The local `completedLessonsSignal` retains the lesson for the rest of the session (from the local optimistic effect at lesson-completion time). - Server's `/Lessons/progress` doesn't include the lesson (it's unverified). - → Refresh: nothing seeds the lesson into the completed set; it's locked. ## Acceptance criteria Decide on the right policy and implement it: - **Option A (lenient):** treat `unverified` like `verified` for the local UX — push it into `completedLessonsSignal` from the flush handler. The lesson stays unlocked; the user just doesn't earn associated skills until a future re-attempt grades cleanly. - **Option B (durable):** keep the IDB row when the response is `unverified` (only delete on `verified` + terminal-rejection reasons) so the next page load can re-merge it via `getPendingPassedLessonNumbers()` and the next flush can re-attempt verification. Both paths fix the symptom; A is simpler, B preserves the chance to eventually earn skills. A combination (A for the UX + B for the retry pathway) is also reasonable. Whichever path is chosen, add a regression test in `progress-sync.service.spec.ts` that flushes with an `unverified` item and asserts the chosen behavior. ## Scope hints - `libraries/tools/src/services/csharp-runner/progress-sync.service.ts` — flush deletion policy. - `libraries/tools/src/components/language-runner/language-runner.ts` — `lastFlushItemsSignal` effect (the `else if` branch that splits verified vs rejected). - `libraries/tools/src/services/csharp-runner/progress-sync.service.spec.ts` — coverage.

spikerj commented

2026-06-15 16:27:29 +00:00

Verified this is already resolved by the durable local-completion ledger (the #223 work that postdates this issue) — effectively the issue's Option B.

In ProgressSyncService.flush() (libraries/platform/progress-sync/src/lib/progress-sync.service.ts):

An unverified/accepted item (no per-item error) deletes the outbox row but keeps the completedLessons ledger entry. Only terminal rejections (unknown_lesson, invalid_token, challenge_lesson_wrong_endpoint, state_expired, server_grading_failed) clear the ledger.
getPendingPassedLessonNumbers() unions outbox + ledger, so after a hard refresh the lesson is still reported as passed and LanguageRunner re-seeds completedLessonsSignal. No re-lock.

The ledger entry is written at pass time during enqueue() (for allTestsPassed items), independent of the flush outcome, so the verify window is covered.

Regression coverage already exists in progress-sync.service.spec.ts and is green:

"keeps the lesson passed after the outbox row is flushed away (hard-refresh durability)" — flushes with errors: [] (the unverified/accepted case) and asserts getPendingPassedLessonNumbers still returns the lesson.
"clears the ledger entry when the server terminally rejects the pass (#73)".
"keeps the ledger entry on a TRANSIENT rejection so it can still survive a refresh (#73)".

The lastFlushItemsSignal handler dropping unverified is correct in-session: the lesson stays passed via the optimistic local completion, and an unverified item legitimately shouldn't flip to a verified badge or award skills. No code change needed; closing as resolved. Reopen if a different policy (e.g. Option A's eager verified-badge flip) is desired.

Verified this is already resolved by the durable local-completion ledger (the #223 work that postdates this issue) — effectively the issue's **Option B**. In `ProgressSyncService.flush()` (`libraries/platform/progress-sync/src/lib/progress-sync.service.ts`): - An `unverified`/accepted item (no per-item error) deletes the **outbox** row but **keeps** the `completedLessons` ledger entry. Only *terminal* rejections (`unknown_lesson`, `invalid_token`, `challenge_lesson_wrong_endpoint`, `state_expired`, `server_grading_failed`) clear the ledger. - `getPendingPassedLessonNumbers()` unions outbox + ledger, so after a hard refresh the lesson is still reported as passed and `LanguageRunner` re-seeds `completedLessonsSignal`. No re-lock. The ledger entry is written at pass time during `enqueue()` (for `allTestsPassed` items), independent of the flush outcome, so the verify window is covered. Regression coverage already exists in `progress-sync.service.spec.ts` and is green: - "keeps the lesson passed after the outbox row is flushed away (hard-refresh durability)" — flushes with `errors: []` (the unverified/accepted case) and asserts `getPendingPassedLessonNumbers` still returns the lesson. - "clears the ledger entry when the server terminally rejects the pass (#73)". - "keeps the ledger entry on a TRANSIENT rejection so it can still survive a refresh (#73)". The `lastFlushItemsSignal` handler dropping `unverified` is correct in-session: the lesson stays passed via the optimistic local completion, and an unverified item legitimately shouldn't flip to a verified badge or award skills. No code change needed; closing as resolved. Reopen if a different policy (e.g. Option A's eager verified-badge flip) is desired.

spikerj closed this issue

2026-06-15 16:27:43 +00:00

spikerj referenced this issue

2026-06-20 02:03:19 +00:00

[Bug] SonarQube S2871: Array.sort() without a reliable comparator #241

spikerj referenced this issue

2026-06-20 02:03:22 +00:00

[Bug] SonarQube: assorted single-site reliability bugs (S7497, S7930, S3516, S7727) #243

spikerj referenced this issue

2026-06-20 02:03:57 +00:00

[Tech debt][Epic] SonarQube: frontend HIGH/BLOCKER maintainability smells (~1,675) #250

spikerj referenced this issue