fix: clean up temporary file after table maintenance job execution

[iemejia]

• Resolves [BUG] Table maintenance commands leave orphaned temporary files on disk #247

Summary

The table maintenance command (fab tables optimize / fab tables vacuum) created a temporary file via NamedTemporaryFile(delete=False) to pass job configuration to the job runner, but never removed it after the job completed. The temp file persisted in the system temp directory indefinitely on every invocation.

Problem

# Before: temp file created but never cleaned up
with tempfile.NamedTemporaryFile(delete=False) as temp_file:
    with open(temp_file.name, "w") as fp:
        json.dump(_config, fp)
    args.input = temp_file.name
    jobs.run_command(args)  # temp file left on disk after this returns

Each invocation of the table maintenance command left an orphaned file in the system temp directory (/tmp/ on Linux, %TEMP% on Windows). Over time, this accumulated files containing table maintenance job configuration (table names, schema names, maintenance parameters).

CWE-459: Incomplete Cleanup

Fix

Wrap the job execution in a try/finally block to ensure os.unlink() is called, removing the temp file even if the job fails with an exception.

# After: temp file always cleaned up
with tempfile.NamedTemporaryFile(delete=False) as temp_file:
    with open(temp_file.name, "w") as fp:
        json.dump(_config, fp)
    args.input = temp_file.name
    try:
        jobs.run_command(args)
    finally:
        os.unlink(temp_file.name)

Tests

3 new tests:

• test_exec_command_cleans_up_temp_file — verifies the temp file is removed after successful execution
• test_exec_command_cleans_up_temp_file_on_error — verifies the temp file is removed even when the job raises an exception
• test_exec_command_writes_correct_config_to_temp_file — verifies the config content (table name, schema, parameters) is written correctly

All existing tests pass (488 total).

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds robust cleanup for the temporary job config file used by fab tables opt, and introduces tests to ensure the temp file is removed in both success and failure scenarios.

Changes:

• Ensure the temp config file is always deleted via try/finally in exec_command.
• Add tests validating temp-file cleanup on success and on error.
• Add a test validating the written temp config contents match expected job configuration.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/fabric_cli/commands/tables/fab_tables_opt.py	Ensures temp config file is unlinked even when jobs.run_command raises.
tests/test_core/test_fab_tables_opt.py	Adds coverage for temp file cleanup and correct config serialization.

[iemejia]

Fixed. Wrapped with contextlib.suppress(OSError) (broadened from FileNotFoundError to also cover PermissionError on Windows).

[iemejia]

Fixed. Replaced with pytest.raises(RuntimeError, match="Job failed") so the test fails if the exception is not thrown.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[iemejia]

Fixed. Moved job execution and cleanup outside the with tempfile.NamedTemporaryFile block so the file handle is closed before os.unlink. Captured the path as temp_path = temp_file.name before exiting the with block.

[aviatco]

Please move the try so it starts right after the temp file is created (before the open). Once NamedTemporaryFile(delete=False) returns, the file already exists on disk, so if open or json.dump raises (e.g. a PermissionError/OSError, or a non-serializable config), the file leaks before we ever reach the cleanup.

[aviatco]

each test's name should end with suffix _success/_failure depending on what the test is testing

[aviatco]

missing cases like file open errors (lack permissions for example) or serialization error

[aviatco]

Consider creating a fixture and use it instead of defining it multiple time

[aviatco]

tests of command should be under tests/test_commands and not under tests/test_core.
please move it to the correct path and use the test_command guidelines.