fix: prevent stale errors

[arthurschreiber]

Fixes #1712.

@chdh Mind taking a look?

[claude]

Code Review: PR #1717 - Fix: Prevent Stale Errors

Overview

This PR fixes issue #1712 where prepared statement request objects retain error state between executions. The fix is well-targeted and includes comprehensive test coverage.

---

✅ Code Quality & Best Practices

Strengths:

• Minimal, surgical fix: The change is exactly where it needs to be - in makeRequest() at src/connection.ts:3179, alongside other request state resets (rowCount, rows, rst)
• Excellent test coverage: The new integration test thoroughly validates the fix with a realistic scenario (divide-by-zero error)
• Good test structure: The test properly uses async/await patterns consistent with modern JavaScript practices
• Clear documentation: Test name includes issue reference (GH#1712), making it easy to trace back to the original problem

Code location consistency:
The fix correctly places request.error = undefined with the other request state resets, which logically groups all state initialization together.

---

✅ Potential Bugs or Issues

None identified. The fix is sound:

1. Correct placement: The error reset happens in makeRequest(), which is called by all request execution paths
2. Consistent with existing patterns: Other request properties are already reset here (lines 3176-3178)
3. Preserves error semantics: Later in the code (line 3218), there's request.error ??= error which only sets error if undefined, so clearing it here is safe

---

✅ Performance Considerations

No performance concerns:

• Single assignment operation has negligible overhead
• Fix is in the already-existing request initialization path
• No additional loops, allocations, or expensive operations

---

✅ Security Concerns

No security issues identified.

The fix actually improves reliability by preventing error state confusion, which could theoretically lead to incorrect error handling in application code that reuses prepared statements.

---

✅ Test Coverage

Excellent test coverage:

The new test at test/integration/prepare-execute-statements-test.ts:159-237 validates:

1. ✓ First execution succeeds (baseline)
2. ✓ Second execution fails with expected error (error handling works)
3. ✓ Third execution succeeds without stale error (the actual fix)
4. ✓ Proper cleanup with unprepare and close

Test quality observations:

• Uses realistic SQL error scenario (divide-by-zero)
• Properly tests the edge case of request object reuse
• Includes clear assertions with descriptive messages
• Follows the repository's existing test patterns (async/await with Promises)

---

Minor Suggestions

1. Consider edge case documentation (optional):
While not required, a brief inline comment at src/connection.ts:3179 could help future maintainers:

request.error = undefined;  // Reset error state for request reuse (GH#1712)

2. Test assertion message clarity (very minor):
At line 231, the assertion message could be slightly more specific:

// Current:
assert.strictEqual(results[1], 0, 'Third execution should return 0 (1/2 truncated to int)');

// Suggestion (more precise):
assert.strictEqual(results[2], 0, 'Third execution should return 0 (1/2 truncated to int)');

Wait - actually checking the test logic: results[1] is correct since the second execution (divisor=0) doesn't add a result due to the error. The third execution is indeed results[1]. So this is correct as-is.

---

Summary

Recommendation: ✅ APPROVE

This is a high-quality fix that:

• Correctly identifies and resolves the root cause
• Uses minimal code changes
• Includes comprehensive test coverage
• Follows repository conventions
• Has no security, performance, or correctness concerns

The fix demonstrates good understanding of the codebase architecture by placing the error reset exactly where other request state is initialized. Great work!

---

Related: Closes #1712

[claude]

Code Review - PR #1717: Fix Prevent Stale Errors

Summary

This PR fixes issue #1712 where prepared statements would get stuck in an error state after a failed execution. The fix correctly resets the request.error property at the beginning of each request execution, and includes a comprehensive test to verify the behavior.

Code Quality ✅

Strengths:

• Clean, minimal fix: The one-line change at src/connection.ts:3179 is exactly where it needs to be - in the makeRequest method alongside other request state initialization
• Consistent with existing pattern: The fix follows the same initialization pattern as rowCount, rows, and rst properties (lines 3176-3178)
• Well-documented test: The test at line 159 has clear comments explaining the bug and expected behavior

Test Coverage ✅

Excellent test design:

• Tests the complete lifecycle: success → failure → success → unprepare
• Captures error state after each execution to verify proper cleanup
• Includes the key assertion at line 235 that directly tests the bug fix
• Uses realistic SQL (divide by zero) that can both succeed and fail
• Properly tests that unprepare doesn't inherit the stale error state

Minor suggestion:
The test comment at line 165 says the callback is "called after each execute", but it might be clearer to note it's for both prepare and execute operations. Not critical, just for clarity.

Implementation Analysis ✅

Correctness:
Looking at the flow:

1. makeRequest (line 3158) initializes request state including request.error = undefined
2. During execution, errors can be set via request.error = error (lines 2659, 2932, 2954, etc.)
3. At request completion (line 3779), the callback receives sqlRequest.error
4. The fix ensures a clean slate for each execution ✓

Why this works:

• The error property is checked at line 3779: sqlRequest.callback(sqlRequest.error, ...)
• Token handlers check !this.request.error before setting errors (lines 496, 531 in token/handler.ts)
• Resetting to undefined at request start prevents old errors from persisting

Performance Considerations ✅

No performance concerns:

• Single property assignment has negligible overhead
• No additional allocations or computations
• Consistent with existing initialization code

Security Considerations ✅

No security concerns:

• Proper error handling maintained
• No exposure of sensitive information
• Fix actually improves reliability by preventing error state confusion

Potential Issues / Questions

1. Complete coverage of request types?
The fix is in makeRequest which is called by:

• execSql / execSqlBatch
• prepare / execute / unprepare
• callProcedure
• newBulkLoad

All code paths are covered ✓

2. Edge case consideration:
What happens if request.error is intentionally set before calling execute? This seems unlikely/inappropriate, but the new behavior would clear it. Looking at the codebase, this doesn't appear to be a valid use case, so this is fine.

Recommendations

Required: None - the PR is ready to merge as-is.

Optional enhancement ideas (not blockers):

1. Consider adding a brief code comment at line 3179 explaining why error is reset: // Clear any stale error state from previous executions
2. The test result assertion at line 236 expects 0 but the comment says "1/2 truncated to int" - verify this is correct for SQL Server integer division (it should be 0, which matches the assertion)

Conclusion

This is a well-crafted fix that:

• ✅ Addresses the root cause correctly
• ✅ Includes comprehensive test coverage
• ✅ Follows existing code patterns
• ✅ Has no performance or security concerns
• ✅ Handles all request types consistently

Recommendation: Approve and merge 🚀

Great work identifying and fixing this subtle state management bug!

[claude]

Code Review

Thank you for this fix! I've reviewed the changes and they correctly address the stale error state issue reported in #1712. Here's my detailed feedback:

---

Positive Aspects

1. Root Cause Fix: The fix correctly addresses the core issue by resetting request.error = undefined in the makeRequest method (src/connection.ts:3179), which is called for all request types including execute().

2. Consistency with Existing Pattern: This aligns perfectly with how other request properties are reset (rowCount, rows, rst) at the same location, making it maintainable and predictable.

3. Comprehensive Test Coverage: The test at test/integration/prepare-execute-statements-test.ts:159-235 is excellent:

   • Tests the exact scenario from the bug report (three executions with error in the middle)
   • Uses realistic SQL error (divide by zero)
   • Validates both success and error cases
   • Includes good comments explaining the bug's behavior

4. Low Risk: The change is minimal (1 line) and surgical, reducing the risk of introducing regressions.

---

Observations and Minor Issues

1. Test Structure Issue (Bug in test)

The test has a scoping problem at line 168-179. The execute helper function is defined before the request variable is declared (line 192), which means it's capturing an undefined variable. This will cause a runtime error.

Current order:

• Line 168: execute() defined here - references 'request' and 'results'
• Line 191: request declared here
• Line 195: results declared here

Fix needed: Move the execute function definition to after both request and results are declared (after line 198).

2. Error Message Assertion

Line 218 uses assert.include to check for "Divide by zero" in the error message. This works, but SQL Server's actual error message is "Divide by zero error encountered." - consider using the exact string or making it more flexible depending on SQL Server version differences.

---

Additional Considerations

3. Documentation

Consider whether this subtle behavior should be documented. Users might wonder why errors don't persist across executions, though this is likely the expected/desired behavior.

4. Other Request Methods

The fix in makeRequest covers execSql, execute, callProcedure, and newBulkLoad. Have you verified that unprepare (which also had the issue per the bug report) goes through this code path? Looking at the code, unprepare should also call makeRequest, so this should be covered, but confirming with a test would be valuable.

5. Edge Case: Multiple Errors

From src/token/handler.ts:398-400, I see that when multiple errors occur, request.error is set to an AggregateError. Resetting to undefined handles this correctly, but it's worth noting that any error aggregation from a previous request is properly cleared.

---

Recommendation

Overall: Approve with minor fix required

The fix is correct and well-tested, but the test will fail in its current form due to the variable scoping issue. Please fix the order of declarations before merging.

Required changes:

• Move execute function definition after request and results declarations

Optional improvements:

• Consider tightening the error message assertion
• Add a comment explaining why request.error needs to be reset

Once the scoping issue is fixed, this PR is ready to merge!

[claude]

Code Review

Thank you for this PR! This is a well-crafted fix for issue #1712. Here's my detailed review:

✅ Code Quality & Best Practices

Excellent:

• The fix is minimal and surgical - adding just one line (request.error = undefined) in the exact right place (src/connection.ts:3179)
• The placement is perfect: resetting error alongside other request state (rowCount, rows, rst) in makeRequest() ensures consistent state management
• Follows the existing codebase pattern where request properties are reset at the start of each request execution

Test Quality:

• Comprehensive integration test that validates the exact bug scenario from issue Prepared request gets stuck in error state #1712
• Tests three executions: success → error → success, which is the critical sequence
• Clear, descriptive comments explain each execution step
• Follows the callback-based pattern used throughout the test file (good consistency)
• Proper cleanup with unprepare() and close()

✅ Bug Fix Correctness

The fix correctly addresses the root cause:

• Problem: When reusing a prepared statement, request.error persisted across executions
• Solution: Reset request.error = undefined in makeRequest(), matching how rowCount, rows, and rst are reset
• The fix works for all request types since makeRequest() is the common entry point

Verified by searching the codebase - request.error is set in 10 locations (token handlers, timeouts, various error scenarios), so resetting it at the start of each request is the right approach.

✅ Performance Considerations

No performance concerns:

• Single assignment operation with negligible overhead
• No additional allocations or complex logic
• Placement in hot path is unavoidable and appropriate

✅ Security Concerns

No security issues identified:

• The fix prevents error state leakage, which is actually a security improvement
• Ensures errors don't incorrectly propagate across executions
• No introduction of new attack vectors

✅ Test Coverage

Strengths:

• Integration test covers the exact reported bug scenario
• Validates both the error case and the critical "error should not persist" case
• Tests with realistic SQL operations (divide by zero)
• Assertions are specific and descriptive

Minor Suggestions:

1. Result array indexing: On line 234, the test uses results[1] for the third execution's result. This works because the second execution (divide by zero) doesn't produce a row, but it's a bit subtle. Consider adding a comment:

   // Note: results[1] is from the third execution; second execution produced no row due to error
   assert.strictEqual(results[1], 0, 'Third execution should return 0 (1/2 truncated to int)');

2. Error message assertion: Line 230 uses assert.include() which is good, but SQL Server error messages can vary slightly. This should be fine for integration tests.

3. Edge case: Consider testing the unprepare() scenario explicitly since the issue description mentions it also failed with stale errors. The current test calls unprepare() but doesn't verify it succeeds without error. You could add:

   connection.unprepare(request);
   // Verify unprepare doesn't fail with stale error by checking the end event succeeds

   Though the current implementation is probably sufficient since the test would fail if unprepare had an error.

📋 Additional Observations

1. Commit history: The PR shows good iterative development (async/await → callback-based refactoring), and the final callback-based version is the right choice for consistency with the rest of the test file.

2. Line 3218 context: I noticed the codebase uses the nullish coalescing assignment operator request.error ??= error on line 3218, which is a good pattern to avoid overwriting existing errors. The fix at line 3179 correctly resets before this logic runs.

3. BulkLoad: The fix applies to both Request and BulkLoad types (both handled by makeRequest), which is correct since both share the same error property pattern.

✅ Recommendation

Approve with minor suggestions. The fix is correct, minimal, and well-tested. The suggestions above are truly minor enhancements and don't block merging.

The PR effectively solves the reported issue and prevents stale error state from affecting subsequent prepared statement executions. Great work! 🎉

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.78%. Comparing base (d49317d) to head (ff14d7d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1717      +/-   ##
==========================================
- Coverage   79.92%   79.78%   -0.14%     
==========================================
  Files          90       90              
  Lines        4887     4888       +1     
  Branches      920      920              
==========================================
- Hits         3906     3900       -6     
- Misses        676      681       +5     
- Partials      305      307       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chdh]

Calling close() without waiting for the unprepare() to complete cancels the unprepare() immediatelly so that is has no effect. While unprepare is not necessary before a close anyway, the test case is not entirely correct in this form and it's a bad example for the users.

[arthurschreiber]

@claude Can you fix this?

[claude]

Claude finished @arthurschreiber's task in 1m 27s —— View job

---

Fixed test to wait for unprepare() before close()

I've fixed the issue where close() was called without waiting for unprepare() to complete.

Todo List:

• Read the test file to understand current implementation
• Fix the test to properly chain unprepare() and close()
• Commit and push the changes

Changes made:

• Modified the test at test/integration/prepare-execute-statements-test.ts:199-202 to listen for the requestCompleted event before calling close()
• This ensures unprepare() completes before the connection is closed
• Prevents the unprepare operation from being cancelled prematurely

The fix follows the same event-driven pattern used throughout the test file, where operations are chained using requestCompleted event handlers. The test now properly waits for unprepare to finish before closing, making it a correct example for users.

Commit: ff14d7d

[arthurschreiber]

@chdh Claude fixed this, but looking at how the prepare/unprepare API is supposed to be used, it makes very little sense to me. 😟

Looks like the callback to new Request is called when the request is unprepared? But how is the user supposed to understand the difference between a request being unprepared and a request being executed?

This seems to be a major design flaw in the current API - prepare, execute, unprepare, execSqlBatch, execSql all should take their own callbacks, and new Request should not even have a callback set.

Also I guess there should be a PreparedRequest object that's returned by prepare and that can be passed into execute and unprepare.

Or do I misunderstand the current API? Do you think the "future" API I'm laying out above is not going in the right direction?

[arthurschreiber]

This seems to be a major design flaw in the current API - prepare, execute, unprepare, execSqlBatch, execSql all should take their own callbacks, and new Request should not even have a callback set.

Probably execSql, execute and execSqlBatch should actually return a Response object which can be iterated over / events will be emitted on.

[chdh]

Looks like the callback to new Request is called when the request is unprepared? But how is the user supposed to understand the difference between a request being unprepared and a request being executed?

This seems to be a major design flaw in the current API - prepare, execute, unprepare, execSqlBatch, execSql all should take their own callbacks, and new Request should not even have a callback set.

Yes, I had exactly this problem when I wrote my promise-based wrapper. I solved it by routing the request completion callback through a function pointer in aPreparedStatement object.

export interface PreparedStatement {
   _dbConnection:                      DbConnection;
   _request:                           Tedious.Request;
   _requestCompletionCallback?:        (err: Error, rowCount: number) => void;
   parmDefs:                           DbParmDef[];
   isPrepared:                         boolean;
   isDisposed:                         boolean;
}

[chdh]

@arthurschreiber thanks. One single added statement solves the whole problem. 😉

I have added a comment for the unit test. Claude also mentioned unprepare() but didn't quite understand how unprepare() works...

I saw that you first attempted a promise-based test. Do you have any concrete plans to add a promisified API for Tedious? I thought about publishing a wrapper-package with a promise-based API for Tedious. I use this already internally for my projects since several years.

Another epochal but due project would be to improve the Tedious API documentation with AI. But it's maybe a bit too early for that?

[arthurschreiber]

I have added a comment for the unit test. Claude also mentioned unprepare() but didn't quite understand how unprepare() works...

This is great feedback! I'm trying to see how far I can push Claude in being helpful maintaining tedious. Sorry for pulling you into this experiment. 😅

I saw that you first attempted a promise-based test. Do you have any concrete plans to add a promisified API for Tedious? I thought about publishing a wrapper-package with a promise-based API for Tedious. I use this already internally for my projects since several years.

I'd love to have a promise based API! So far, I've always failed to find a way to introduce a promise based API and fix all the ugliness / mistakes in the existing API at the same time. I'm wondering if it would make sense to introduce a tedious/promises entry point (similar to how pure-promise based APIs are exposed by NodeJS), and then design a new API from the ground up?

Another epochal but due project would be to improve the Tedious API documentation with AI. But it's maybe a bit too early for that?

Yes, I'd definitely look to investigate in that direction. I started with very basic cleanups over in #1721 - but I'd love to push this even further.

[claude]

Code Review for PR #1717: Fix Prevent Stale Errors

Summary

This PR fixes issue #1712 where prepared request objects retained error state across multiple executions. The fix adds request.error = undefined; to the state reset logic in the makeRequest method, ensuring errors don't persist between requests.

✅ Code Quality and Best Practices

Strengths:

• Correct placement: The fix is placed in makeRequest at src/connection.ts:3179, which is the central location where all request state is reset (alongside rowCount, rows, and rst). This ensures the error state is cleared for ALL request types (execute, prepare, unprepare, callProcedure, etc.).
• Minimal change: The one-line fix follows the principle of minimal intervention - it doesn't over-engineer or refactor unnecessarily.
• Consistent with existing pattern: The fix mirrors the existing state reset logic for other request properties on lines 3176-3178.

✅ Bug Fix Validation

The fix correctly addresses the issue:

• Before: Error state persisted on the request object after a failed execution, causing subsequent executions and unprepare operations to fail with stale errors.
• After: Error state is cleared at the start of each makeRequest call, allowing requests to succeed independently.

✅ Test Coverage

Excellent test design:

• The test at test/integration/prepare-execute-statements-test.ts:159-240 comprehensively validates the fix with a real-world scenario:

  1. Execute with valid input (divisor=1) → should succeed
  2. Execute with invalid input (divisor=0) → should fail with divide-by-zero
  3. Execute with valid input (divisor=2) → should succeed (this would fail before the fix)
  4. Unprepare → should succeed (this would fail before the fix)

• The test uses proper assertion patterns consistent with the codebase:

  • assert.isUndefined() and assert.isDefined() for error checks
  • assert.strictEqual() for value validation
  • assert.include() for error message verification

• Good test hygiene:

  • Clear comments explaining the test scenario
  • Reference to issue number (GH#1712) in test name
  • Proper cleanup with connection.close()
  • Uses requestCompleted events appropriately

✅ Performance Considerations

No performance impact:

• The change adds a single property assignment (request.error = undefined;) during request initialization
• This is negligible compared to the overall cost of database operations
• The assignment happens once per request, same as other state resets

✅ Security Considerations

Security improvement:

• Information disclosure prevention: The fix prevents error objects from leaking between requests, which could potentially expose sensitive error details from one operation to another
• Correct error attribution: Ensures errors are properly attributed to the operations that caused them, improving debugging and security auditing

📝 Suggestions

Minor observation (not blocking):

The fix is in the right place, but you might consider documenting this behavior. The makeRequest method could benefit from a comment explaining that it resets request state to ensure clean execution. For example:

// Reset request state to ensure clean execution
request.rowCount! = 0;
request.rows! = [];
request.rst! = [];
request.error = undefined;

However, this is truly optional - the code is clear enough as-is.

✅ Overall Assessment

LGTM! 🎯

This is a well-crafted fix that:

• ✅ Correctly identifies and fixes the root cause
• ✅ Places the fix in the optimal location (centralized state reset)
• ✅ Includes comprehensive test coverage
• ✅ Follows existing code patterns and conventions
• ✅ Has no negative side effects
• ✅ Improves security by preventing error state leakage

The PR is ready to merge. Great work on the thorough test case that validates the fix!

---

Review generated by Claude Code

[chdh]

.I'm wondering if it would make sense to introduce a tedious/promises entry point (similar to how pure-promise based APIs are exposed by NodeJS), and then design a new API from the ground up?

Yes, I think that these kind of problems can be solved by writing a sophisticated promise-based wrapper around the old API. It's also important to ensure that the promise of an active request is ended when the whole connection is closed.

Another problem is that, in some places in the API, values can be passed as arrays or as Record<string, any> objects or both (as in connection.execBulkLoad()). I would like to allow both cases everywhere, even passing the parameter values for a prepared statement as an array. I don't know whether options.useColumnNames could be changed easily on the fly, but there is certainly a solution somehow.

Such a wrapper could be written and published as a separate NPM package as a proof of concept. Later it could be integrated into the main package as tedious/promises.

[arthurschreiber]

Another problem is that, in some places in the API, values can be passed as arrays or as Record<string, any> objects or both (as in connection.execBulkLoad()). I would like to allow both cases everywhere, even passing the parameter values for a prepared statement as an array. I don't know whether options.useColumnNames could be changed easily on the fly, but there is certainly a solution somehow.

IIRC, options.useColumnNames is only relevant for output, not for input? 🤔 Either way, I'd like to get rid of options.useColumnNames too. Having a configuration like this that changes the result types in such a drastic fashion makes typing really awkward. I'd much rather have a method like Response.rowsAsArrays (returning AsyncIterable<unknown[]>) and one like Response.rowsAsObjects (returning AsyncIterable<Record<string, unknown>>) so that users of the library can decide on demand how to iterate over the result and have at least some type checking support.