# Recover IMPORT INTO Jobs With Service-Owned Group Key

## Analysis

| Area | Impact | Verification |
| --- | --- | --- |
| ImportIntoTask creation | New tasks must ignore caller-provided `group_key` and persist `import-into-<importID>` after the DB ID is allocated. | Create-task unit test confirms persisted and returned group key. |
| IMPORT INTO SQL | Existing SQL builder must continue adding `GROUP_KEY` whenever `imp.GroupKey` is non-empty. | SQL builder unit test confirms generated group key appears in the SQL. |
| 8173 recovery | A failed async submit task with TiDB active-job error must recover Job_ID through `SHOW IMPORT JOBS WHERE group_key = ...`. | Unit tests cover detection, row matching, no-match, and ambiguous-match behavior. |
| Existing tasks | Existing rows keep their stored `group_key`; recovery only runs when a group key is available. | Recovery helper returns no match when group key is empty. |

Symbols to keep:
- `buildImportIntoSQL`
- existing async task state handling
- existing `SHOW IMPORT JOB <id>` progress sync

Symbols to add:
- service-owned group-key builder
- 8173 detection helper
- import jobs query/matching helpers

## Execution Steps

1. Update `CreateImportIntoTask` to create the entity inside a DB transaction, then set `GroupKey` to `import-into-<id>` and persist it before commit.
   - Success criteria: newly returned import task includes the generated group key and DB row stores the same value.

2. Update import-admin response conversion to include `GroupKey`.
   - Success criteria: API response exposes the generated key for operators/UI.

3. Add recovery helpers in `internal/model/import/import_into.go`.
   - Detect only TiDB 8173 / active-job messages as recoverable.
   - Query `SHOW IMPORT JOBS WHERE group_key = '<generated-key>'`.
   - Parse returned rows by column name.
   - Match `group_key + target_table` only.
   - Do not use `Data_Source` for recovery filtering: TiDB may normalize or rewrite the URI, and within one service-owned `group_key` there should not be multiple submissions targeting the same table.
   - Return a recovered Job_ID only when exactly one candidate remains.

4. Wire recovery into the import-into submit-state handler for failed submit tasks.
   - Success criteria: an 8173 submit failure writes the recovered Job_ID and avoids marking the subtask failed.
   - Failure criteria: no match or ambiguous match preserves the original submit failure behavior.

5. Add focused tests for create-task group key generation, SQL building, 8173 detection, and recovery matching.
   - Verification commands:
     - `go test ./internal/model/import/... ./internal/openapi/service/import/...`
     - `make imp-unit-test`
     - `make build-dataflow-darwin`

## Risk Assessment

- Query support risk: assumes target TiDB supports `SHOW IMPORT JOBS WHERE group_key = ...`.
- Matching risk: incorrect Job_ID recovery is worse than surfacing the 8173 error, so ambiguous results must fail closed.
- Recovery scope invariant: a single service-owned `group_key` must not contain multiple `IMPORT INTO` submissions for the same target table, otherwise `group_key + target_table` is no longer sufficient for unique recovery.
- Compatibility risk: caller-provided group keys are ignored for new tasks; existing tasks keep existing DB values.

## Follow-Up Decisions

1. Recovery matching stays on `group_key + target_table`.
   - Rationale: the service owns `group_key`, and a task's subtask set should not contain duplicate target-table submissions within the same group.
   - Rationale: `Data_Source` is not stable enough to serve as a recovery key because TiDB may normalize the URI or inject extra query parameters such as `external-id`.

2. The pre-submit target-table empty check is intentionally not restored.
   - Rationale: the async `IMPORT INTO` submission path already returns the target-table-not-empty failure, so maintaining a separate precheck only duplicates behavior and creates dead helper code/tests.

## Temporary Validation Follow-Up

To validate the production recovery path end-to-end, the temporary agent-side simulation must drop the in-memory submit task only once per `task_id`.

1. First submit:
   - TiDB accepts the detached `IMPORT INTO`.
   - agent-ng gets the `Job_ID`, deletes the in-memory task, and logs the simulated loss.
   - Success criteria: service logs `failed to get import-into job task`.

2. Second submit retry:
   - service recreates the same submit task and re-executes the same SQL.
   - TiDB should return `ERROR 8173` because the original import job is still active.
   - agent-ng must keep this failed task result in memory so service can read the 8173 message.
   - Success criteria: service logs `recovered import-into job id after active-job conflict`.

3. Verification commands:
   - `go test ./dataflow-agent-ng/agent/grpc -run 'Test(CreateImportIntoJobTask|GetImportIntoJobTaskFailed|CreateImportIntoJobTaskRejectsNonImportSQL)'`
   - `go test ./internal/model/import -run 'Test(BuildImportIntoSQLIncludesGeneratedGroupKey|IsRecoverableActiveImportJobErr|RecoverImportIntoJobIDWithExec|FindRecoverableImportIntoJobIDFailsClosed|ResolveImportIntoJobTaskStateAndApplyResult)'`

## State-Machine Orchestration Follow-Up

The recovery work widened the IMPORT INTO phase state machine. Resolver unit tests alone are not enough because phase handlers can still wire those decisions in the wrong order or abort a subtask loop too early.

1. Keep `Preparing` orchestration explicit.
   - Success criteria: after file discovery completes, schema preparation still runs in the same polling round; incomplete schema preparation waits without skipping future steps.
   - Verification: `go test ./internal/model/import -run 'TestRunImportIntoPreparingStepsOrchestration'`

2. Make `Importing` and `Canceling` loops complete every subtask before resolving the phase.
   - Success criteria: one subtask progress-sync error does not stop later subtasks from being processed in the same round.
   - Verification: `go test ./internal/model/import -run 'TestRunImportIntoImportingProgressSyncOrchestration|TestRunImportIntoCancelingProgressSyncOrchestration'`

3. Keep resolver tests and orchestration tests together.
   - Success criteria: resolver logic and phase wiring are both covered.
   - Verification:
     - `go test ./internal/model/import -run 'Test(ResolveImportIntoPreparingDecision|ResolveImportIntoScalingUpDecision|ResolveImportIntoImportingDecision|ResolveImportIntoCancelingDecision)'`
     - `make app-unit-test-local`

## PEM Rotation Follow-Up

The SQL executor refactor introduced a regression: `pem_changed` now clears the cached cluster but returns the sentinel error directly, while submit, recovery, progress-sync, and cancel paths all expect transparent retry semantics.

1. Restore executor-local retry on `pem_changed`.
   - Success criteria: a single `execute` call refreshes the PEM, rebuilds the encrypted cluster, and retries `ExecuteSQL` without exposing `agent.PEMChangedError` to callers.

2. Replace the regression test with a retry-success test.
   - Success criteria: the first `ExecuteSQL` response can set `pem_changed=true`, the second response succeeds, and the test proves two exec attempts happened in one call.

3. Verification:
   - `go test ./internal/model/import -run 'TestImportIntoSQLExecutorExecuteRetriesPEMChanged|TestSyncImportIntoProgressWithExecReturnsOriginalError'`
   - `make app-unit-test-local`