From 0ec3bf9467699acf0b335dd2e17af0dfc2050e1a Mon Sep 17 00:00:00 2001 From: Nandan Rao Date: Wed, 8 Jul 2026 10:03:08 -0400 Subject: [PATCH] fix(replybot): clear stale error/wait/retries on state transitions Transient fields (error, wait, waitStart, retries) were leaking into states where they no longer apply. For example, a participant in WAIT_EXTERNAL_EVENT that received a MACHINE_REPORT error would keep the stale wait object in the new ERROR state. Similarly, END and QOUT transitions retained error/retry context from prior states. This is visible to users via the Monitor tab's raw state_json viewer and pollutes the error_tag/fb_error_code computed columns used by StatesList filtering. Changes to apply(): - RESPOND: also clear wait/waitStart (already cleared error/retries) - RESPOND_AGAIN (REDO): clear error/wait, keep retries for Dean backoff - WAIT_RESPONSE (QOUT): clear error/retries - HANDOFF: clear error/retries - WAIT_EXTERNAL_EVENT: clear error/retries - END: clear error/wait/retries - BLOCKED: clear wait/waitStart (keep error/retries) - ERROR: clear wait/waitStart (keep error/retries) Invariant enforced: transient fields exist only in their owning states - error: ERROR, BLOCKED - wait/waitStart: WAIT_EXTERNAL_EVENT - retries: RESPONDING, ERROR, BLOCKED Added 6 logs-based tests covering real reachable transitions. Updated 1 existing test that was asserting on the stale waitStart leak. --- replybot/lib/typewheels/machine.js | 45 ++++++++++++++--- replybot/lib/typewheels/machine.test.js | 67 ++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 10 deletions(-) diff --git a/replybot/lib/typewheels/machine.js b/replybot/lib/typewheels/machine.js index 342b4978..31fad70e 100644 --- a/replybot/lib/typewheels/machine.js +++ b/replybot/lib/typewheels/machine.js @@ -579,7 +579,7 @@ function apply(state, output) { case 'RESPOND': - // NOTE: by removing errors/retries on RESPOND, we are "resetting" + // NOTE: by removing errors/retries/waits on RESPOND, we are "resetting" // our retry-on-error process (and exponential backoff) whenever // the user responds. I think this is reasonable. But it's implicit here. return { @@ -591,6 +591,8 @@ function apply(state, output) { previousOutput: output, error: undefined, // remove error when responding retries: undefined, // remove retries when responding + wait: undefined, // remove wait when user responds + waitStart: undefined, // remove waitStart when user responds qa: updateQA(state.qa, update(output)) } @@ -610,7 +612,9 @@ function apply(state, output) { return { ...state, ...output.stateUpdate, - state: 'RESPONDING' + state: 'RESPONDING', + error: undefined, // remove stale error on retry (keep retries for backoff) + wait: undefined, // remove stale wait on retry } @@ -628,7 +632,9 @@ function apply(state, output) { return { ...state, state: 'QOUT', - question: output.question + question: output.question, + error: undefined, // question sent, no error context + retries: undefined, // question sent, no retry context } case 'HANDOFF': @@ -639,7 +645,9 @@ function apply(state, output) { question: output.question, wait: output.wait, externalEvents: output.externalEvents || state.externalEvents, - waitStart: output.waitStart + waitStart: output.waitStart, + error: undefined, // entering wait, clear prior error + retries: undefined, // entering wait, clear prior retries } case 'WAIT_EXTERNAL_EVENT': @@ -650,21 +658,42 @@ function apply(state, output) { question: output.question, wait: output.wait, externalEvents: output.externalEvents || state.externalEvents, - waitStart: output.waitStart + waitStart: output.waitStart, + error: undefined, // entering/continuing wait, clear prior error + retries: undefined, // entering/continuing wait, clear prior retries } case 'END': - return { ...state, state: 'END', question: output.question } + return { + ...state, + state: 'END', + question: output.question, + error: undefined, // completed, no error context + wait: undefined, // completed, no wait context + retries: undefined, // completed, no retry context + } case 'BLOCKED': - return { ...state, state: 'BLOCKED', error: output.error } + return { + ...state, + state: 'BLOCKED', + error: output.error, + wait: undefined, // blocked, clear prior wait + waitStart: undefined, // blocked, clear prior waitStart + } case 'UNBLOCK': return { ...state, ...output.stateUpdate } case 'ERROR': - return { ...state, state: 'ERROR', error: output.error } + return { + ...state, + state: 'ERROR', + error: output.error, + wait: undefined, // errored, clear prior wait + waitStart: undefined, // errored, clear prior waitStart + } default: return state diff --git a/replybot/lib/typewheels/machine.test.js b/replybot/lib/typewheels/machine.test.js index 9d48abac..83fe9558 100644 --- a/replybot/lib/typewheels/machine.test.js +++ b/replybot/lib/typewheels/machine.test.js @@ -451,7 +451,7 @@ describe('getState', () => { }) - it('Responds while waiting with response and repeats with old waitstart', () => { + it('Responds while waiting with response and gets fresh waitstart on new wait', () => { const wait = { type: 'timeout', value: '2 days' } @@ -464,7 +464,7 @@ describe('getState', () => { ] const state = getState(log) state.state.should.equal('WAIT_EXTERNAL_EVENT') - state.waitStart.should.equal(5) + state.waitStart.should.equal(10) }) @@ -827,6 +827,69 @@ describe('getState', () => { actions.messages[0].recipient.one_time_notif_token.should.equal('FOOBAR') actions.messages[0].message.text.should.equal('barbaz') }) + + describe('transient field cleanup', () => { + + it('clears error on REDO but keeps retries accumulating', () => { + const report = synthetic({ type: 'machine_report', value: { error: { tag: 'INTERNAL', code: 'FOO' } } }) + const redo = synthetic({ type: 'redo' }) + const log = [referral, echo, text, report, redo] + const state = getState(log) + state.state.should.equal('RESPONDING') + should.not.exist(state.error) + state.retries.should.exist + state.retries.length.should.equal(1) + }) + + it('clears error and retries when user responds from ERROR state', () => { + const report = synthetic({ type: 'machine_report', value: { error: { tag: 'INTERNAL', code: 'FOO' } } }) + const log = [referral, echo, text, report, text] + const state = getState(log) + state.state.should.equal('RESPONDING') + should.not.exist(state.error) + should.not.exist(state.retries) + }) + + it('clears wait when entering ERROR from WAIT_EXTERNAL_EVENT', () => { + const wait = { type: 'timeout', value: '2 days' } + const report = synthetic({ type: 'machine_report', value: { error: { tag: 'INTERNAL', code: 'FOO' } } }) + const log = [referral, echo, text, _echo({ wait, ref: 'bar' }), report] + const state = getState(log) + state.state.should.equal('ERROR') + should.not.exist(state.wait) + should.not.exist(state.waitStart) + state.error.code.should.equal('FOO') + }) + + it('clears wait when entering BLOCKED from WAIT_EXTERNAL_EVENT', () => { + const wait = { type: 'timeout', value: '2 days' } + const fbReport = synthetic({ type: 'machine_report', value: { error: { tag: 'FB', code: 200, message: 'foo' } } }) + const log = [referral, echo, text, _echo({ wait, ref: 'bar' }), fbReport] + const state = getState(log) + state.state.should.equal('BLOCKED') + should.not.exist(state.wait) + should.not.exist(state.waitStart) + }) + + it('clears error, wait, and retries on END', () => { + const report = synthetic({ type: 'machine_report', value: { error: { tag: 'INTERNAL', code: 'FOO' } } }) + const log = [referral, echo, text, report, tyEcho] + const state = getState(log) + state.state.should.equal('END') + should.not.exist(state.error) + should.not.exist(state.wait) + should.not.exist(state.retries) + }) + + it('clears wait and waitStart when user responds while waiting', () => { + const wait = { type: 'timeout', value: '2 days' } + const log = [referral, echo, delivery, text, _echo({ wait, ref: 'bar' }), text] + const state = getState(log) + state.state.should.equal('RESPONDING') + should.not.exist(state.wait) + should.not.exist(state.waitStart) + }) + }) })